From 272cdd80b3f93b61facaffa39a886dcd368980d9 Mon Sep 17 00:00:00 2001 From: Christopher Fitzner Date: Tue, 14 Jan 2025 16:20:46 -0800 Subject: [PATCH] CC-30971: update state with allowlist api responses Previously the object the api returned for created and updated allowlist entries was ignored when updating the terraform state. This meant that defaults set on the server side and returned were not correctly being set and could cause state inconsistencies in case they were different. Specifically there was a case where not setting an initial value for the create would return an empty string. The empty string caused a state consistency error with the unset value. To fix this we now set the state based on the response. As part of this the code was reworked to be more similar to how other resources are structured. Tests were added to show that creating an allowlist without a name now succeeds. Also, as part of this change, instances of allowList were renamed to allowlist for consistency. --- CHANGELOG.md | 5 + docs/resources/allow_list.md | 2 +- internal/provider/allowlist_resource.go | 117 +++++++----- internal/provider/allowlist_resource_test.go | 188 +++++++++++++------ 4 files changed, 201 insertions(+), 111 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c44e7d1..35868e81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +### Fixed + +- Fix a state consistency error that occurred when an allowlist was created + without a name. + ## [1.11.0] - 2024-12-09 ### Changed diff --git a/docs/resources/allow_list.md b/docs/resources/allow_list.md index b7208ece..92d5cdbf 100644 --- a/docs/resources/allow_list.md +++ b/docs/resources/allow_list.md @@ -36,7 +36,7 @@ resource "cockroach_allow_list" "vpn" { ### Optional -- `name` (String) Name of this allowlist entry. If not set explicitly, this value does not sync with the server. +- `name` (String) Name of this allowlist entry. If left unset, it will inherit a server-side default. ### Read-Only diff --git a/internal/provider/allowlist_resource.go b/internal/provider/allowlist_resource.go index b5a3c839..07c1576e 100644 --- a/internal/provider/allowlist_resource.go +++ b/internal/provider/allowlist_resource.go @@ -34,7 +34,7 @@ import ( // clusterID:ip/mask const ( - allowListIDFmt = "%s:%s/%d" + allowlistIDFmt = "%s:%s/%d" allowlistEntryRegex = `(([0-9]{1,3}\.){3}[0-9]{1,3})\/([0-9]|[1-2][0-9]|3[0-2])` // Allowlist entries are lightweight, so we can use a large limit. allowlistEntryPaginationLimit = 500 @@ -42,11 +42,11 @@ const ( var allowlistIDRegex = regexp.MustCompile(fmt.Sprintf("^(%s):%s$", uuidRegexString, allowlistEntryRegex)) -type allowListResource struct { +type allowlistResource struct { provider *provider } -func (r *allowListResource) Schema( +func (r *allowlistResource) Schema( _ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse, ) { resp.Schema = schema.Schema{ @@ -83,7 +83,7 @@ func (r *allowListResource) Schema( "name": schema.StringAttribute{ Computed: true, Optional: true, - Description: "Name of this allowlist entry. If not set explicitly, this value does not sync with the server.", + Description: "Name of this allowlist entry. If left unset, it will inherit a server-side default.", }, "id": schema.StringAttribute{ Computed: true, @@ -96,7 +96,7 @@ func (r *allowListResource) Schema( } } -func (r *allowListResource) Configure( +func (r *allowlistResource) Configure( _ context.Context, req resource.ConfigureRequest, resp *resource.ConfigureResponse, ) { if req.ProviderData == nil { @@ -109,13 +109,13 @@ func (r *allowListResource) Configure( } } -func (r *allowListResource) Metadata( +func (r *allowlistResource) Metadata( _ context.Context, req resource.MetadataRequest, resp *resource.MetadataResponse, ) { resp.TypeName = req.ProviderTypeName + "_allow_list" } -func (r *allowListResource) Create( +func (r *allowlistResource) Create( ctx context.Context, req resource.CreateRequest, resp *resource.CreateResponse, ) { if r.provider == nil || !r.provider.configured { @@ -123,33 +123,22 @@ func (r *allowListResource) Create( return } - var entry AllowlistEntry - diags := req.Plan.Get(ctx, &entry) + var plan AllowlistEntry + diags := req.Plan.Get(ctx, &plan) resp.Diagnostics.Append(diags...) - // Create a unique ID (required by terraform framework) by combining - // the cluster ID and full CIDR address. - entry.ID = types.StringValue(fmt.Sprintf( - allowListIDFmt, entry.ClusterId.ValueString(), - entry.CidrIp.ValueString(), entry.CidrMask.ValueInt64())) - if resp.Diagnostics.HasError() { return } - var allowList = client.AllowlistEntry{ - CidrIp: entry.CidrIp.ValueString(), - CidrMask: int32(entry.CidrMask.ValueInt64()), - Ui: entry.Ui.ValueBool(), - Sql: entry.Sql.ValueBool(), - } - - if IsKnown(entry.Name) { - name := entry.Name.ValueString() - allowList.Name = &name - } + clusterID := plan.ClusterId.ValueString() traceAPICall("AddAllowlistEntry") - _, _, err := r.provider.service.AddAllowlistEntry(ctx, entry.ClusterId.ValueString(), &allowList) + allowlist, _, err := r.provider.service.AddAllowlistEntry(ctx, clusterID, &client.AllowlistEntry{ + CidrIp: plan.CidrIp.ValueString(), + CidrMask: int32(plan.CidrMask.ValueInt64()), + Ui: plan.Ui.ValueBool(), + Sql: plan.Sql.ValueBool(), + }) if err != nil { resp.Diagnostics.AddError( "Error adding allowed IP range", @@ -158,14 +147,38 @@ func (r *allowListResource) Create( return } - diags = resp.State.Set(ctx, entry) + var state AllowlistEntry + loadAllowlistEntryToTerraformState(clusterID, allowlist, &state) + diags = resp.State.Set(ctx, state) resp.Diagnostics.Append(diags...) if resp.Diagnostics.HasError() { return } } -func (r *allowListResource) Read( +func generateAllowlistID(clusterID, cidrIP string, cidrMask int32) string { + return fmt.Sprintf(allowlistIDFmt, clusterID, cidrIP, cidrMask) +} + +func loadAllowlistEntryToTerraformState(clusterID string, entry *client.AllowlistEntry, state *AllowlistEntry) { + // Create a unique ID (required by terraform framework) by combining + // the cluster ID and full CIDR address. + state.ID = types.StringValue(generateAllowlistID(clusterID, entry.GetCidrIp(), entry.GetCidrMask())) + state.ClusterId = types.StringValue(clusterID) + state.CidrIp = types.StringValue(entry.GetCidrIp()) + state.CidrMask = types.Int64Value(int64(entry.GetCidrMask())) + state.Ui = types.BoolValue(entry.GetUi()) + state.Sql = types.BoolValue(entry.GetSql()) + // Name is incorrectly specified in the API as a pointer and will never be + // nil but we'll handle it here anyways. + if entry.Name == nil { + state.Name = types.StringValue("") + } else { + state.Name = types.StringValue(entry.GetName()) + } +} + +func (r *allowlistResource) Read( ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse, ) { if r.provider == nil || !r.provider.configured { @@ -181,6 +194,8 @@ func (r *allowListResource) Read( return } + clusterID := state.ClusterId.ValueString() + // Since the state may have come from an import, we need to retrieve // the actual entry list and make sure this one is in there. var page string @@ -188,7 +203,7 @@ func (r *allowListResource) Read( for { traceAPICall("ListAllowlistEntries") apiResp, httpResp, err := r.provider.service.ListAllowlistEntries( - ctx, state.ClusterId.ValueString(), &client.ListAllowlistEntriesOptions{ + ctx, clusterID, &client.ListAllowlistEntriesOptions{ PaginationPage: &page, PaginationLimit: &limit, }, @@ -214,17 +229,11 @@ func (r *allowListResource) Read( return } - for _, entry := range apiResp.GetAllowlist() { - if entry.GetCidrIp() == state.CidrIp.ValueString() && - int64(entry.GetCidrMask()) == state.CidrMask.ValueInt64() { - // Update flags in case they've changed externally. - state.Sql = types.BoolValue(entry.GetSql()) - state.Ui = types.BoolValue(entry.GetUi()) - if entry.Name == nil { - state.Name = types.StringNull() - } else { - state.Name = types.StringValue(entry.GetName()) - } + // Find the allowlist entry this resource represents. + for _, respEntry := range apiResp.GetAllowlist() { + respEntryID := generateAllowlistID(clusterID, respEntry.GetCidrIp(), respEntry.GetCidrMask()) + if respEntryID == state.ID.ValueString() { + loadAllowlistEntryToTerraformState(clusterID, &respEntry, &state) diags = resp.State.Set(ctx, &state) resp.Diagnostics.Append(diags...) return @@ -246,7 +255,7 @@ func (r *allowListResource) Read( resp.State.RemoveResource(ctx) } -func (r *allowListResource) Update( +func (r *allowlistResource) Update( ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse, ) { // Get plan values @@ -265,21 +274,21 @@ func (r *allowListResource) Update( return } - clusterId := plan.ClusterId.ValueString() - entryCIDRIp := plan.CidrIp.ValueString() + clusterID := plan.ClusterId.ValueString() + entryCIDRIP := plan.CidrIp.ValueString() entryCIDRMask := int32(plan.CidrMask.ValueInt64()) - updatedAllowList := client.AllowlistEntry1{ + update := &client.AllowlistEntry1{ Ui: plan.Ui.ValueBool(), Sql: plan.Sql.ValueBool(), } if IsKnown(plan.Name) { - updatedAllowList.Name = ptr(plan.Name.ValueString()) + update.Name = ptr(plan.Name.ValueString()) } traceAPICall("UpdateAllowlistEntry") - _, _, err := r.provider.service.UpdateAllowlistEntry( - ctx, clusterId, entryCIDRIp, entryCIDRMask, &updatedAllowList) + updatedEntry, _, err := r.provider.service.UpdateAllowlistEntry( + ctx, clusterID, entryCIDRIP, entryCIDRMask, update) if err != nil { resp.Diagnostics.AddError( "Error updating network allowlist", @@ -288,14 +297,15 @@ func (r *allowListResource) Update( return } - diags = resp.State.Set(ctx, plan) + loadAllowlistEntryToTerraformState(clusterID, updatedEntry, &state) + diags = resp.State.Set(ctx, state) resp.Diagnostics.Append(diags...) if resp.Diagnostics.HasError() { return } } -func (r *allowListResource) Delete( +func (r *allowlistResource) Delete( ctx context.Context, req resource.DeleteRequest, resp *resource.DeleteResponse, ) { var state AllowlistEntry @@ -324,7 +334,7 @@ func (r *allowListResource) Delete( resp.State.RemoveResource(ctx) } -func (r *allowListResource) ImportState( +func (r *allowlistResource) ImportState( ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse, ) { // Since an allowlist entry is uniquely identified by three fields: the cluster ID, @@ -340,6 +350,9 @@ func (r *allowListResource) ImportState( } // We can swallow this error because it's already been regex-validated. mask, _ = strconv.Atoi(matches[4]) + + // This is a partial state object but includes all the fields which are used + // in the subsequent READ operation to fill out the rest of the state. entry := AllowlistEntry{ ClusterId: types.StringValue(matches[1]), CidrIp: types.StringValue(matches[2]), @@ -350,5 +363,5 @@ func (r *allowListResource) ImportState( } func NewAllowlistResource() resource.Resource { - return &allowListResource{} + return &allowlistResource{} } diff --git a/internal/provider/allowlist_resource_test.go b/internal/provider/allowlist_resource_test.go index f6770f8e..44e32b3c 100644 --- a/internal/provider/allowlist_resource_test.go +++ b/internal/provider/allowlist_resource_test.go @@ -104,6 +104,9 @@ func TestIntegrationAllowlistEntryResource(t *testing.T) { Sql: false, Ui: false, } + newEntryWithEmptyStringName := newEntry + newEntryWithEmptyStringName.Name = ptr("") + if os.Getenv(CockroachAPIKey) == "" { os.Setenv(CockroachAPIKey, "fake") } @@ -182,83 +185,87 @@ func TestIntegrationAllowlistEntryResource(t *testing.T) { cluster := c.finalCluster entry := c.entry + entryWithNilName := entry + entryWithNilName.Name = nil + entryWithEmptyStringName := entry + entryWithEmptyStringName.Name = ptr("") - // Create + // Step: Initial creation, entry with nil name s.EXPECT().CreateCluster(gomock.Any(), gomock.Any()). Return(&cluster, nil, nil) + + // Calls to GetBackupConfiguration and GetCluster are called many + // times throughout the test. The requests and responses don't + // change so these are set to return AnyTimes() to simplify the + // remainder of the tests. s.EXPECT().GetBackupConfiguration(gomock.Any(), clusterID). Return(initialBackupConfig, httpOk, nil).AnyTimes() s.EXPECT().GetCluster(gomock.Any(), clusterID). - Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil) - s.EXPECT().AddAllowlistEntry(gomock.Any(), clusterID, &entry).Return(&entry, nil, nil) - // Called by testAllowlistEntryExists + Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil).AnyTimes() + s.EXPECT().AddAllowlistEntry(gomock.Any(), clusterID, &entryWithNilName).Return(&entryWithEmptyStringName, nil, nil) s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()).Return( - &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, entry}}, nil, nil) + &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, entryWithEmptyStringName}}, nil, nil).Times(2) + + // Step: Update to empty string name should keep the name as empty string + s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()).Return( + &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, entryWithEmptyStringName}}, nil, nil).Times(3) + + // Step: Update to empty string name again to show no churn + s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()).Return( + &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, entryWithEmptyStringName}}, nil, nil).Times(3) - // Update 1 + // Step: Set an actual value for name // The OpenAPI generator does something weird when part of an object lives in the URL // and the rest in the request body, and it winds up as a partial object. + entryForUpdate := &client.AllowlistEntry1{ + Name: entry.Name, + Sql: entry.Sql, + Ui: entry.Ui, + } + s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()).Return( + &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, entryWithEmptyStringName}}, nil, nil) + s.EXPECT().UpdateAllowlistEntry(gomock.Any(), clusterID, entry.CidrIp, entry.CidrMask, entryForUpdate). + Return(&entry, &http.Response{Status: http.StatusText(http.StatusOK)}, nil) + s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()). + Return(&client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, entry}}, nil, nil).Times(2) + + // Step: Update other fields newEntryForUpdate := &client.AllowlistEntry1{ Name: newEntry.Name, Sql: newEntry.Sql, Ui: newEntry.Ui, } - // GetCluster and ListAllowListEntries called for each allow list entry - s.EXPECT().GetCluster(gomock.Any(), clusterID). - Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil).Times(2) s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()).Return( - &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, entry}}, nil, nil).Times(2) - // One allowlist was updated + &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, entry}}, nil, nil) s.EXPECT().UpdateAllowlistEntry(gomock.Any(), clusterID, entry.CidrIp, entry.CidrMask, newEntryForUpdate). Return(&newEntry, &http.Response{Status: http.StatusText(http.StatusOK)}, nil) - // Called by testAllowlistEntryExists - s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()). - Return(&client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntry}}, nil, nil) - - // Update 2 - nil name (this should not make an update) - s.EXPECT().GetCluster(gomock.Any(), clusterID). - Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil) - s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()). - Return(&client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntry}}, nil, nil) - s.EXPECT().GetCluster(gomock.Any(), clusterID). - Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil) s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()). Return(&client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntry}}, nil, nil).Times(2) - // Update 3 - add name back but make it empty string (this should an update) - newEntry2Update := &client.AllowlistEntry1{ + // Step: Remove name as a managed attribute + s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()).Return( + &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntry}}, nil, nil).Times(3) + + // Step: Add name back as empty string + newEntryWithEmptyNameForUpdate := &client.AllowlistEntry1{ Name: ptr(""), Sql: newEntry.Sql, Ui: newEntry.Ui, } - newEntry2 := newEntry - newEntry2.Name = ptr("") - - // Two pairs of these, one for each allowlist - s.EXPECT().GetCluster(gomock.Any(), clusterID). - Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil).Times(2) - s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()). - Return(&client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntry}}, nil, nil).Times(2) - // One allowlist was updated - s.EXPECT().UpdateAllowlistEntry(gomock.Any(), clusterID, entry.CidrIp, entry.CidrMask, newEntry2Update). - Return(&newEntry2, &http.Response{Status: http.StatusText(http.StatusOK)}, nil) - // Called by testAllowlistEntryExists + s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()).Return( + &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntry}}, nil, nil) + s.EXPECT().UpdateAllowlistEntry(gomock.Any(), clusterID, entry.CidrIp, entry.CidrMask, newEntryWithEmptyNameForUpdate). + Return(&newEntryWithEmptyStringName, &http.Response{Status: http.StatusText(http.StatusOK)}, nil) s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()). - Return(&client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntry2}}, nil, nil) + Return(&client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntryWithEmptyStringName}}, nil, nil).Times(2) - // Update 4 - Update to empty string again (no churn) - // Two pairs of these, one for each allowlist - s.EXPECT().GetCluster(gomock.Any(), clusterID). - Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil).Times(2) - s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()). - Return(&client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntry2}}, nil, nil).Times(3) + // Step: Update to empty string name again to show no churn + s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()).Return( + &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntryWithEmptyStringName}}, nil, nil).Times(3) - // Delete - s.EXPECT().GetCluster(gomock.Any(), clusterID). - Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil) + // Deletion s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()).Return( - &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntry2}}, nil, nil). - Times(2) + &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, newEntryWithEmptyStringName}}, nil, nil) s.EXPECT().DeleteAllowlistEntry(gomock.Any(), clusterID, entry.CidrIp, entry.CidrMask) s.EXPECT().DeleteCluster(gomock.Any(), clusterID) @@ -282,10 +289,14 @@ func testAllowlistEntryResource( var clusterResourceName string var allowlistEntryResourceConfigFn func(string, *client.AllowlistEntry) string var uiVal string - entryWithNilName := newEntry + entryWithNilName := entry entryWithNilName.Name = nil - entryWithEmptyStringName := newEntry + entryWithEmptyStringName := entry entryWithEmptyStringName.Name = ptr("") + newEntryWithNilName := newEntry + newEntryWithNilName.Name = nil + newEntryWithEmptyStringName := newEntry + newEntryWithEmptyStringName.Name = ptr("") if isServerless { clusterResourceName = serverlessClusterResourceName @@ -301,7 +312,54 @@ func testAllowlistEntryResource( PreCheck: func() { testAccPreCheck(t) }, ProtoV6ProviderFactories: testAccProtoV6ProviderFactories, Steps: []resource.TestStep{ + // Start with a Nil named entry to show that state correctly writes + // the default response value of the entry which is empty string { + PreConfig: func() { + traceMessageStep("Initial creation, entry with nil name") + }, + Config: allowlistEntryResourceConfigFn(clusterName, &entryWithNilName), + Check: resource.ComposeTestCheckFunc( + testAllowlistEntryExists(resourceName, clusterResourceName), + resource.TestCheckResourceAttr(resourceName, "name", ""), + resource.TestCheckResourceAttrSet(resourceName, "cidr_ip"), + resource.TestCheckResourceAttrSet(resourceName, "cidr_mask"), + resource.TestCheckResourceAttr(resourceName, "ui", uiVal), + resource.TestCheckResourceAttr(resourceName, "sql", "true"), + ), + }, + { + PreConfig: func() { + traceMessageStep("Update to empty string name should keep the name as empty string") + }, + Config: allowlistEntryResourceConfigFn(clusterName, &entryWithEmptyStringName), + Check: resource.ComposeTestCheckFunc( + testAllowlistEntryExists(resourceName, clusterResourceName), + resource.TestCheckResourceAttr(resourceName, "name", ""), + resource.TestCheckResourceAttrSet(resourceName, "cidr_ip"), + resource.TestCheckResourceAttrSet(resourceName, "cidr_mask"), + resource.TestCheckResourceAttr(resourceName, "ui", uiVal), + resource.TestCheckResourceAttr(resourceName, "sql", "true"), + ), + }, + { + PreConfig: func() { + traceMessageStep("Update to empty string name again to show no churn") + }, + Config: allowlistEntryResourceConfigFn(clusterName, &entryWithEmptyStringName), + Check: resource.ComposeTestCheckFunc( + testAllowlistEntryExists(resourceName, clusterResourceName), + resource.TestCheckResourceAttr(resourceName, "name", ""), + resource.TestCheckResourceAttrSet(resourceName, "cidr_ip"), + resource.TestCheckResourceAttrSet(resourceName, "cidr_mask"), + resource.TestCheckResourceAttr(resourceName, "ui", uiVal), + resource.TestCheckResourceAttr(resourceName, "sql", "true"), + ), + }, + { + PreConfig: func() { + traceMessageStep("Set an actual value for name") + }, Config: allowlistEntryResourceConfigFn(clusterName, &entry), Check: resource.ComposeTestCheckFunc( testAllowlistEntryExists(resourceName, clusterResourceName), @@ -313,6 +371,9 @@ func testAllowlistEntryResource( ), }, { + PreConfig: func() { + traceMessageStep("Update other fields") + }, Config: allowlistEntryResourceConfigFn(clusterName, &newEntry), Check: resource.ComposeTestCheckFunc( testAllowlistEntryExists(resourceName, clusterResourceName), @@ -323,9 +384,14 @@ func testAllowlistEntryResource( resource.TestCheckResourceAttr(resourceName, "sql", "false"), ), }, - // Update that removes name from the resource + // Update that removes name from the resource, Since a name is + // already set on the server, that name will be maintained because + // we've removed management of this value from terraform. { - Config: allowlistEntryResourceConfigFn(clusterName, &entryWithNilName), + PreConfig: func() { + traceMessageStep("Remove name as a managed attribute") + }, + Config: allowlistEntryResourceConfigFn(clusterName, &newEntryWithNilName), Check: resource.ComposeTestCheckFunc( testAllowlistEntryExists(resourceName, clusterResourceName), // Expect that the server name is not affected if the name is not explicitly set @@ -336,12 +402,13 @@ func testAllowlistEntryResource( resource.TestCheckResourceAttr(resourceName, "sql", "false"), ), }, - // Update to empty string name { - Config: allowlistEntryResourceConfigFn(clusterName, &entryWithEmptyStringName), + PreConfig: func() { + traceMessageStep("Add name back as empty string") + }, + Config: allowlistEntryResourceConfigFn(clusterName, &newEntryWithEmptyStringName), Check: resource.ComposeTestCheckFunc( testAllowlistEntryExists(resourceName, clusterResourceName), - // Expect that the server name is not affected if the name is not explicitly set resource.TestCheckResourceAttr(resourceName, "name", ""), resource.TestCheckResourceAttrSet(resourceName, "cidr_ip"), resource.TestCheckResourceAttrSet(resourceName, "cidr_mask"), @@ -349,12 +416,13 @@ func testAllowlistEntryResource( resource.TestCheckResourceAttr(resourceName, "sql", "false"), ), }, - // Update to empty string name again to show no churn { - Config: allowlistEntryResourceConfigFn(clusterName, &entryWithEmptyStringName), + PreConfig: func() { + traceMessageStep("Update to empty string name again to show no churn") + }, + Config: allowlistEntryResourceConfigFn(clusterName, &newEntryWithEmptyStringName), Check: resource.ComposeTestCheckFunc( testAllowlistEntryExists(resourceName, clusterResourceName), - // Expect that the server name is not affected if the name is not explicitly set resource.TestCheckResourceAttr(resourceName, "name", ""), resource.TestCheckResourceAttrSet(resourceName, "cidr_ip"), resource.TestCheckResourceAttrSet(resourceName, "cidr_mask"), @@ -363,9 +431,13 @@ func testAllowlistEntryResource( ), }, { + PreConfig: func() { + traceMessageStep("Deletion") + }, ResourceName: resourceName, ImportState: true, ImportStateVerify: true, + Check: traceEndOfPlan(), }, }, })