From c5800ad9b484b25ff6a755e2c43d3d0c988012dd Mon Sep 17 00:00:00 2001 From: Christopher Fitzner Date: Fri, 31 May 2024 15:20:26 -0700 Subject: [PATCH] fix allowlist_resource churn on empty name Previously, allowlist.name attribute was not marked as computed so values from the server would not set the state in case it was unused. The server side default of "" would then conflict with a nil value in the state and show that it needed to be updated on every apply. We now mark the allowlist name as computed so that it can update the state when it is not set. Additionally we stop sending the value on updates unless it explicitly set in the resource. --- CHANGELOG.md | 7 ++ docs/resources/allow_list.md | 2 +- internal/provider/allowlist_resource.go | 8 +- internal/provider/allowlist_resource_test.go | 124 +++++++++++++++++-- internal/provider/cluster_resource_test.go | 4 - internal/provider/utils.go | 4 + 6 files changed, 130 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4773a6ae..86cb95c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.7.1] - 2024-06-05 + +## Fixed + +- Fixed apply churn when the optional name attribute in the allowlist resource was + not included. + ## [1.7.0] - 2024-05-17 ## Added diff --git a/docs/resources/allow_list.md b/docs/resources/allow_list.md index b7a1ed36..cc61d2c8 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. +- `name` (String) Name of this allowlist entry. If not set explicitly, this value does not sync with the server. ### Read-Only diff --git a/internal/provider/allowlist_resource.go b/internal/provider/allowlist_resource.go index 777a3057..8c89d63c 100644 --- a/internal/provider/allowlist_resource.go +++ b/internal/provider/allowlist_resource.go @@ -81,8 +81,9 @@ func (r *allowListResource) Schema( Description: "Set to 'true' to allow SQL connections from this CIDR range.", }, "name": schema.StringAttribute{ + Computed: true, Optional: true, - Description: "Name of this allowlist entry.", + Description: "Name of this allowlist entry. If not set explicitly, this value does not sync with the server.", }, "id": schema.StringAttribute{ Computed: true, @@ -268,11 +269,12 @@ func (r *allowListResource) Update( entryCIDRIp := plan.CidrIp.ValueString() entryCIDRMask := int32(plan.CidrMask.ValueInt64()) - name := plan.Name.ValueString() updatedAllowList := client.AllowlistEntry1{ Ui: plan.Ui.ValueBool(), Sql: plan.Sql.ValueBool(), - Name: &name, + } + if IsKnown(plan.Name) { + updatedAllowList.Name = ptr(plan.Name.ValueString()) } traceAPICall("UpdateAllowlistEntry") diff --git a/internal/provider/allowlist_resource_test.go b/internal/provider/allowlist_resource_test.go index 22bfdfde..0ca974c1 100644 --- a/internal/provider/allowlist_resource_test.go +++ b/internal/provider/allowlist_resource_test.go @@ -187,14 +187,13 @@ func TestIntegrationAllowlistEntryResource(t *testing.T) { s.EXPECT().CreateCluster(gomock.Any(), gomock.Any()). Return(&cluster, nil, nil) s.EXPECT().GetCluster(gomock.Any(), clusterID). - Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil). - Times(4) + Return(&cluster, &http.Response{Status: http.StatusText(http.StatusOK)}, nil) s.EXPECT().AddAllowlistEntry(gomock.Any(), clusterID, &entry).Return(&entry, nil, nil) + // Called by testAllowlistEntryExists s.EXPECT().ListAllowlistEntries(gomock.Any(), clusterID, gomock.Any()).Return( - &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, entry}}, nil, nil). - Times(3) + &client.ListAllowlistEntriesResponse{Allowlist: []client.AllowlistEntry{otherEntry, entry}}, nil, nil) - // Update + // Update 1 // 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. newEntryForUpdate := &client.AllowlistEntry1{ @@ -202,13 +201,62 @@ func TestIntegrationAllowlistEntryResource(t *testing.T) { 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 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(3) + 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{ + 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, newEntry2}}, nil, nil) + + // 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) // Delete + 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, newEntry2}}, nil, nil). + Times(2) s.EXPECT().DeleteAllowlistEntry(gomock.Any(), clusterID, entry.CidrIp, entry.CidrMask) s.EXPECT().DeleteCluster(gomock.Any(), clusterID) @@ -232,6 +280,11 @@ func testAllowlistEntryResource( var clusterResourceName string var allowlistEntryResourceConfigFn func(string, *client.AllowlistEntry) string var uiVal string + entryWithNilName := newEntry + entryWithNilName.Name = nil + entryWithEmptyStringName := newEntry + entryWithEmptyStringName.Name = ptr("") + if isServerless { clusterResourceName = serverlessClusterResourceName allowlistEntryResourceConfigFn = getTestAllowlistEntryResourceConfigForServerless @@ -268,6 +321,45 @@ func testAllowlistEntryResource( resource.TestCheckResourceAttr(resourceName, "sql", "false"), ), }, + // Update that removes name from the resource + { + Config: allowlistEntryResourceConfigFn(clusterName, &entryWithNilName), + 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", *newEntry.Name), + resource.TestCheckResourceAttrSet(resourceName, "cidr_ip"), + resource.TestCheckResourceAttrSet(resourceName, "cidr_mask"), + resource.TestCheckResourceAttr(resourceName, "ui", "false"), + resource.TestCheckResourceAttr(resourceName, "sql", "false"), + ), + }, + // Update to empty string name + { + Config: allowlistEntryResourceConfigFn(clusterName, &entryWithEmptyStringName), + 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"), + resource.TestCheckResourceAttr(resourceName, "ui", "false"), + resource.TestCheckResourceAttr(resourceName, "sql", "false"), + ), + }, + // Update to empty string name again to show no churn + { + Config: allowlistEntryResourceConfigFn(clusterName, &entryWithEmptyStringName), + 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"), + resource.TestCheckResourceAttr(resourceName, "ui", "false"), + resource.TestCheckResourceAttr(resourceName, "sql", "false"), + ), + }, { ResourceName: resourceName, ImportState: true, @@ -316,6 +408,11 @@ func testAllowlistEntryExists(resourceName, clusterResourceName string) resource func getTestAllowlistEntryResourceConfigForDedicated( clusterName string, entry *client.AllowlistEntry, ) string { + nameAttribute := "" + if entry.Name != nil { + nameAttribute = fmt.Sprintf("name = \"%s\"\n", *entry.Name) + } + return fmt.Sprintf(` resource "cockroach_cluster" "dedicated" { name = "%s" @@ -330,19 +427,24 @@ resource "cockroach_cluster" "dedicated" { }] } resource "cockroach_allow_list" "network_list" { - name = "%s" + %s cidr_ip = "%s" cidr_mask = %d sql = %v ui = %v cluster_id = cockroach_cluster.dedicated.id } -`, clusterName, *entry.Name, entry.CidrIp, entry.CidrMask, entry.Sql, entry.Ui) +`, clusterName, nameAttribute, entry.CidrIp, entry.CidrMask, entry.Sql, entry.Ui) } func getTestAllowlistEntryResourceConfigForServerless( clusterName string, entry *client.AllowlistEntry, ) string { + nameAttribute := "" + if entry.Name != nil { + nameAttribute = fmt.Sprintf("name = \"%s\"\n", *entry.Name) + } + return fmt.Sprintf(` resource "cockroach_cluster" "serverless" { name = "%s" @@ -355,12 +457,12 @@ resource "cockroach_cluster" "serverless" { }] } resource "cockroach_allow_list" "network_list" { - name = "%s" + %s cidr_ip = "%s" cidr_mask = %d sql = %v ui = %v cluster_id = cockroach_cluster.serverless.id } -`, clusterName, *entry.Name, entry.CidrIp, entry.CidrMask, entry.Sql, entry.Ui) +`, clusterName, nameAttribute, entry.CidrIp, entry.CidrMask, entry.Sql, entry.Ui) } diff --git a/internal/provider/cluster_resource_test.go b/internal/provider/cluster_resource_test.go index ba550dc6..3c066bb4 100644 --- a/internal/provider/cluster_resource_test.go +++ b/internal/provider/cluster_resource_test.go @@ -953,7 +953,3 @@ func TestClusterSchemaInSync(t *testing.T) { dAttrs := dSchema.Schema.Attributes CheckSchemaAttributesMatch(t, rAttrs, dAttrs) } - -func ptr[T any](in T) *T { - return &in -} diff --git a/internal/provider/utils.go b/internal/provider/utils.go index 0f791ea0..5cf38766 100644 --- a/internal/provider/utils.go +++ b/internal/provider/utils.go @@ -169,6 +169,10 @@ func IsKnown[T Knowable](t T) bool { return !t.IsUnknown() && !t.IsNull() } +func ptr[T any](in T) *T { + return &in +} + // traceAPICall is a helper for debugging which api calls are happening when to // make it easier to determine for understanding what the provider framework is // doing and for determining which calls will need to be mocked in our tests.