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

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
 

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

internal/provider/allowlist_resource_test.go

@@ -187,28 +187,76 @@ 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{
 				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
 			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)
 }

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

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.