Prevent duplicate clusters ending up in Terraform state (#264)

Author: onematchfox

argocd/resource_argocd_cluster.go

@@ -90,17 +90,20 @@ func resourceArgoCDClusterCreate(ctx context.Context, d *schema.ResourceData, me
 
 	client := *server.ClusterClient
 
+	// Need a full lock here to avoid race conditions between List existing clusters and creating a new one
+	tokenMutexClusters.Lock()
+
 	// Cluster are unique by "server address" so we should check there is no existing cluster with this address before
-	tokenMutexClusters.RLock()
 	existingClusters, err := client.List(ctx, &clusterClient.ClusterQuery{
 		Id: &clusterClient.ClusterID{
 			Type:  "server",
 			Value: cluster.Server, // TODO: not used by backend, upstream bug ?
 		},
 	})
-	tokenMutexClusters.RUnlock()
 
 	if err != nil {
+		tokenMutexClusters.Unlock()
+
 		return []diag.Diagnostic{
 			{
 				Severity: diag.Error,
@@ -115,6 +118,8 @@ func resourceArgoCDClusterCreate(ctx context.Context, d *schema.ResourceData, me
 	if len(existingClusters.Items) > 0 {
 		for _, existingCluster := range existingClusters.Items {
 			if rtrimmedServer == strings.TrimRight(existingCluster.Server, "/") {
+				tokenMutexClusters.Unlock()
+
 				return []diag.Diagnostic{
 					{
 						Severity: diag.Error,
@@ -125,9 +130,8 @@ func resourceArgoCDClusterCreate(ctx context.Context, d *schema.ResourceData, me
 		}
 	}
 
-	tokenMutexClusters.Lock()
 	c, err := client.Create(ctx, &clusterClient.ClusterCreateRequest{
-		Cluster: cluster, Upsert: true})
+		Cluster: cluster, Upsert: false})
 	tokenMutexClusters.Unlock()
 
 	if err != nil {

argocd/resource_argocd_cluster_test.go

@@ -273,99 +273,22 @@ func TestAccArgoCDCluster_metadata(t *testing.T) {
 	})
 }
 
-func TestAccArgoCDCluster_uniqueByServerEvenWithDifferentNames(t *testing.T) {
-	name := acctest.RandString(10)
-
+func TestAccArgoCDCluster_invalidSameServer(t *testing.T) {
 	resource.Test(t, resource.TestCase{
 		PreCheck:          func() { testAccPreCheck(t); testAccPreCheckFeatureSupported(t, featureProjectScopedClusters) },
 		ProviderFactories: testAccProviders,
 		Steps: []resource.TestStep{
 			{
-				Config:      testAccArgoCDClusterTwiceWithSameServer(name, name+"d"),
+				Config:      testAccArgoCDClusterTwiceWithSameServer(),
 				ExpectError: regexp.MustCompile("cluster with server address .* already exists"),
-				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr(
-						"argocd_cluster.cluster_one",
-						"info.0.connection_state.0.status",
-						"Successful",
-					),
-					resource.TestCheckResourceAttr(
-						"argocd_cluster.cluster_one",
-						"config.0.tls_client_config.0.insecure",
-						"true",
-					),
-					resource.TestCheckResourceAttr(
-						"argocd_cluster.cluster_one",
-						"name",
-						name,
-					),
-				),
 			},
-		},
-	})
-}
-
-func TestAccArgoCDCluster_uniqueByServer(t *testing.T) {
-	name := acctest.RandString(10)
-
-	resource.Test(t, resource.TestCase{
-		PreCheck:          func() { testAccPreCheck(t); testAccPreCheckFeatureSupported(t, featureProjectScopedClusters) },
-		ProviderFactories: testAccProviders,
-		Steps: []resource.TestStep{
 			{
 				Config:      testAccArgoCDClusterTwiceWithSameServerNoNames(),
 				ExpectError: regexp.MustCompile("cluster with server address .* already exists"),
-				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr(
-						"argocd_cluster.cluster_one",
-						"info.0.connection_state.0.status",
-						"Successful",
-					),
-					resource.TestCheckResourceAttr(
-						"argocd_cluster.cluster_one",
-						"config.0.tls_client_config.0.insecure",
-						"true",
-					),
-					resource.TestCheckResourceAttr(
-						"argocd_cluster.cluster_one",
-						"name",
-						name,
-					),
-				),
 			},
-		},
-	})
-}
-
-func TestAccArgoCDCluster_uniqueByServerTrimmed(t *testing.T) {
-	name := acctest.RandString(10)
-
-	resource.Test(t, resource.TestCase{
-		PreCheck:          func() { testAccPreCheck(t); testAccPreCheckFeatureSupported(t, featureProjectScopedClusters) },
-		ProviderFactories: testAccProviders,
-		Steps: []resource.TestStep{
 			{
-				Config: testAccArgoCDClusterTwiceWithSameServerNoNamesTrimmed(
-					"https://kubernetes.default.svc.cluster.local",
-					"https://kubernetes.default.svc.cluster.local/"),
+				Config:      testAccArgoCDClusterTwiceWithSameLogicalServer(),
 				ExpectError: regexp.MustCompile("cluster with server address .* already exists"),
-				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr(
-						"argocd_cluster.cluster_one",
-						"info.0.connection_state.0.status",
-						"Successful",
-					),
-					resource.TestCheckResourceAttr(
-						"argocd_cluster.cluster_one",
-						"config.0.tls_client_config.0.insecure",
-						"true",
-					),
-					resource.TestCheckResourceAttr(
-						"argocd_cluster.cluster_one",
-						"name",
-						name,
-					),
-				),
 			},
 		},
 	})
@@ -486,11 +409,11 @@ resource "argocd_cluster" "cluster_metadata" {
 `
 }
 
-func testAccArgoCDClusterTwiceWithSameServer(clusterName, clusterName2 string) string {
-	return fmt.Sprintf(`
-resource "argocd_cluster" "cluster_one" {
+func testAccArgoCDClusterTwiceWithSameServer() string {
+	return `
+resource "argocd_cluster" "cluster_one_same_server" {
   server = "https://kubernetes.default.svc.cluster.local"
-  name   = "%s"
+  name   = "foo"
   config {
     # Uses Kind's bootstrap token whose ttl is 24 hours after cluster bootstrap.
     bearer_token = "abcdef.0123456789abcdef"
@@ -499,23 +422,22 @@ resource "argocd_cluster" "cluster_one" {
     }
   }
 }
-resource "argocd_cluster" "cluster_two" {
+resource "argocd_cluster" "cluster_two_same_server" {
   server = "https://kubernetes.default.svc.cluster.local"
-  name   = "%s"
+  name   = "bar"
   config {
     # Uses Kind's bootstrap token whose ttl is 24 hours after cluster bootstrap.
     bearer_token = "abcdef.0123456789abcdef"
     tls_client_config {
       insecure = true
     }
   }
-}
-`, clusterName, clusterName2)
+}`
 }
 
 func testAccArgoCDClusterTwiceWithSameServerNoNames() string {
 	return `
-resource "argocd_cluster" "cluster_one" {
+resource "argocd_cluster" "cluster_one_no_name" {
   server = "https://kubernetes.default.svc.cluster.local"
   config {
     # Uses Kind's bootstrap token whose ttl is 24 hours after cluster bootstrap.
@@ -525,7 +447,7 @@ resource "argocd_cluster" "cluster_one" {
     }
   }
 }
-resource "argocd_cluster" "cluster_two" {
+resource "argocd_cluster" "cluster_two_no_name" {
   server = "https://kubernetes.default.svc.cluster.local"
   config {
     # Uses Kind's bootstrap token whose ttl is 24 hours after cluster bootstrap.
@@ -538,11 +460,11 @@ resource "argocd_cluster" "cluster_two" {
 `
 }
 
-func testAccArgoCDClusterTwiceWithSameServerNoNamesTrimmed(server, server2 string) string {
-	return fmt.Sprintf(`
-resource "argocd_cluster" "cluster_one" {
+func testAccArgoCDClusterTwiceWithSameLogicalServer() string {
+	return `
+resource "argocd_cluster" "cluster_with_trailing_slash" {
   name = "server"
-  server = "%s"
+  server = "https://kubernetes.default.svc.cluster.local/"
   config {
     # Uses Kind's bootstrap token whose ttl is 24 hours after cluster bootstrap.
     bearer_token = "abcdef.0123456789abcdef"
@@ -551,18 +473,17 @@ resource "argocd_cluster" "cluster_one" {
     }
   }
 }
-resource "argocd_cluster" "cluster_two" {
+resource "argocd_cluster" "cluster_with_no_trailing_slash" {
   name = "server"
-  server = "%s"
+  server = "https://kubernetes.default.svc.cluster.local"
   config {
     # Uses Kind's bootstrap token whose ttl is 24 hours after cluster bootstrap.
     bearer_token = "abcdef.0123456789abcdef"
     tls_client_config {
       insecure = true
     }
   }
-}
-`, server, server2)
+}`
 }
 
 func testAccArgoCDClusterMetadata_addLabels(clusterName string) string {