feat: Network_peering long-running operation improvements (#3515)

* remove logs

* use WithoutTimeout funcs

* timeouts

* delete_on_create_timeout

* changelog

* more generic doc so can be used for any resource

* rename to errWait

* TestAccNetworkNetworkPeering_timeouts

* change doc

* add clarification

* also change error message

* fix TestAccNetworkRSNetworkPeering_Azure

* don't use default

* Revert "fix TestAccNetworkRSNetworkPeering_Azure"

This reverts commit ca9f4e0.

* no need to exclude delete_on_create_timeout in import if Default is not used

* use d.GetOkExists to distinguish empty from zero (false) value

* Update .changelog/3515.txt

Co-authored-by: Javier Armendáriz <[email protected]>

* Update internal/common/cleanup/handle_timeout.go

Co-authored-by: Javier Armendáriz <[email protected]>

* improve changelog

* use new context in cleanup

---------

Co-authored-by: Javier Armendáriz <[email protected]>

Author: lantoli

.changelog/3515.txt

@@ -0,0 +1,7 @@
+```release-note:enhancement
+resource/mongodbatlas_network_peering: Adds `timeouts` attribute for create, update and delete operations
+```
+
+```release-note:enhancement
+resource/mongodbatlas_network_peering: Adds `delete_on_create_timeout` attribute to indicate whether to delete the resource if its creation times out
+```

docs/resources/network_peering.md

@@ -330,6 +330,8 @@ resource "mongodbatlas_network_peering" "test" {
 * `project_id` - (Required) The unique ID for the MongoDB Atlas project.
 * `container_id` - (Required) Unique identifier of the MongoDB Atlas container for the provider (GCP) or provider/region (AWS, AZURE). You can create an MongoDB Atlas container using the network_container resource or it can be obtained from the cluster returned values if a cluster has been created before the first container.
 * `provider_name` - (Required) Cloud provider to whom the peering connection is being made. (Possible Values `AWS`, `AZURE`, `GCP`).
+* `timeouts`- (Optional) The duration of time to wait for the resource to be created, updated, or deleted. The default timeout is `1h`. The timeout value is defined by a signed sequence of decimal numbers with a time unit suffix such as: `1h45m`, `300s`, `10m`, etc. The valid time units are:  `ns`, `us` (or `µs`), `ms`, `s`, `m`, `h`. Learn more about timeouts [here](https://www.terraform.io/plugin/sdkv2/resources/retries-and-customizable-timeouts).
+* `delete_on_create_timeout`- (Optional) Flag that indicates whether to delete the resource if creation times out. Default is `true`. When Terraform apply fails, it returns immediately without waiting for cleanup to complete. If you suspect a transient error, wait before retrying to allow resource deletion to finish.
 
 **AWS ONLY:**
 

internal/common/cleanup/on_timeout.go renamed to internal/common/cleanup/handle_timeout.go

@@ -7,12 +7,31 @@ import (
 	"time"
 
 	"github.com/hashicorp/terraform-plugin-framework/diag"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry"
 )
 
 const (
 	CleanupWarning = "Failed to create resource. Will run cleanup due to the operation timing out"
 )
 
+// HandleCreateTimeout helps to implement Create in long-running operations.
+// It deletes the resource if the creation times out and `delete_on_create_timeout` is enabled.
+// It returns an error with additional information which should be used instead of the original error.
+func HandleCreateTimeout(deleteOnCreateTimeout bool, errWait error, cleanup func(context.Context) error) error {
+	if _, isTimeoutErr := errWait.(*retry.TimeoutError); !isTimeoutErr {
+		return errWait
+	}
+	if !deleteOnCreateTimeout {
+		return errors.Join(errWait, errors.New("cleanup won't be run because delete_on_create_timeout is false"))
+	}
+	errWait = errors.Join(errWait, errors.New("will run cleanup because delete_on_create_timeout is true. If you suspect a transient error, wait before retrying to allow resource deletion to finish"))
+	// cleanup uses a new context as existing one is expired.
+	if errCleanup := cleanup(context.Background()); errCleanup != nil {
+		errWait = errors.Join(errWait, errors.New("cleanup failed: "+errCleanup.Error()))
+	}
+	return errWait
+}
+
 // OnTimeout creates a new context with a timeout and a deferred function that will run `cleanup` when the context hit the timeout (no timeout=no-op).
 // Remember to always call the returned `deferCall` function: `defer deferCall()`.
 // `warningDetail` should have resource identifiable information, for example cluster name and project ID.

internal/common/cleanup/on_timeout_test.go renamed to internal/common/cleanup/handle_timeout_test.go

@@ -4,14 +4,14 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"log"
 	"strings"
 	"time"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
+	"github.com/mongodb/terraform-provider-mongodbatlas/internal/common/cleanup"
 	"github.com/mongodb/terraform-provider-mongodbatlas/internal/common/conversion"
 	"github.com/mongodb/terraform-provider-mongodbatlas/internal/common/validate"
 	"github.com/mongodb/terraform-provider-mongodbatlas/internal/config"
@@ -24,17 +24,24 @@ const (
 	errorPeersRead   = "error reading MongoDB Network Peering Connection (%s): %s"
 	errorPeersDelete = "error deleting MongoDB Network Peering Connection (%s): %s"
 	errorPeersUpdate = "error updating MongoDB Network Peering Connection (%s): %s"
+
+	minTimeout = 10 * time.Second
 )
 
 func Resource() *schema.Resource {
 	return &schema.Resource{
-		CreateContext: resourceCreate,
-		ReadContext:   resourceRead,
-		UpdateContext: resourceUpdate,
-		DeleteContext: resourceDelete,
+		CreateWithoutTimeout: resourceCreate,
+		ReadWithoutTimeout:   resourceRead,
+		UpdateWithoutTimeout: resourceUpdate,
+		DeleteWithoutTimeout: resourceDelete,
 		Importer: &schema.ResourceImporter{
 			StateContext: resourceImportState,
 		},
+		Timeouts: &schema.ResourceTimeout{
+			Create: schema.DefaultTimeout(1 * time.Hour),
+			Update: schema.DefaultTimeout(1 * time.Hour),
+			Delete: schema.DefaultTimeout(1 * time.Hour),
+		},
 		Schema: map[string]*schema.Schema{
 			"project_id": {
 				Type:     schema.TypeString,
@@ -92,7 +99,6 @@ func Resource() *schema.Resource {
 				Type:     schema.TypeString,
 				Computed: true,
 			},
-
 			"atlas_cidr_block": {
 				Type:     schema.TypeString,
 				Optional: true,
@@ -157,6 +163,11 @@ func Resource() *schema.Resource {
 				Type:     schema.TypeString,
 				Computed: true,
 			},
+			"delete_on_create_timeout": { // Don't use Default: true to avoid unplanned changes when upgrading from previous versions.
+				Type:        schema.TypeBool,
+				Optional:    true,
+				Description: "Flag that indicates whether to delete the resource if creation times out. Default is true.",
+			},
 		},
 	}
 }
@@ -244,19 +255,27 @@ func resourceCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.
 	if err != nil {
 		return diag.FromErr(fmt.Errorf(errorPeersCreate, err))
 	}
+	peerID := peer.GetId()
 
 	stateConf := &retry.StateChangeConf{
 		Pending:    []string{"INITIATING", "FINALIZING", "ADDING_PEER", "WAITING_FOR_USER"},
 		Target:     []string{"FAILED", "AVAILABLE", "PENDING_ACCEPTANCE"},
-		Refresh:    resourceRefreshFunc(ctx, peer.GetId(), projectID, peerRequest.GetContainerId(), conn.NetworkPeeringApi),
-		Timeout:    1 * time.Hour,
-		MinTimeout: 10 * time.Second,
-		Delay:      30 * time.Second,
+		Refresh:    resourceRefreshFunc(ctx, peerID, projectID, peerRequest.GetContainerId(), conn.NetworkPeeringApi),
+		Timeout:    d.Timeout(schema.TimeoutCreate),
+		MinTimeout: minTimeout,
+		Delay:      minTimeout,
 	}
-
-	_, err = stateConf.WaitForStateContext(ctx)
-	if err != nil {
-		return diag.FromErr(fmt.Errorf(errorPeersCreate, err))
+	_, errWait := stateConf.WaitForStateContext(ctx)
+	deleteOnCreateTimeout := true // default value when not set
+	if v, ok := d.GetOkExists("delete_on_create_timeout"); ok {
+		deleteOnCreateTimeout = v.(bool)
+	}
+	errWait = cleanup.HandleCreateTimeout(deleteOnCreateTimeout, errWait, func(ctxCleanup context.Context) error {
+		_, _, errCleanup := conn.NetworkPeeringApi.DeletePeeringConnection(ctxCleanup, projectID, peerID).Execute()
+		return errCleanup
+	})
+	if errWait != nil {
+		return diag.Errorf(errorPeersCreate, errWait)
 	}
 
 	d.SetId(conversion.EncodeStateID(map[string]string{
@@ -459,9 +478,9 @@ func resourceUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.
 		Pending:    []string{"INITIATING", "FINALIZING", "ADDING_PEER", "WAITING_FOR_USER"},
 		Target:     []string{"FAILED", "AVAILABLE", "PENDING_ACCEPTANCE"},
 		Refresh:    resourceRefreshFunc(ctx, peerID, projectID, "", conn.NetworkPeeringApi),
-		Timeout:    d.Timeout(schema.TimeoutCreate),
-		MinTimeout: 30 * time.Second,
-		Delay:      1 * time.Minute,
+		Timeout:    d.Timeout(schema.TimeoutUpdate),
+		MinTimeout: minTimeout,
+		Delay:      minTimeout,
 	}
 
 	_, err = stateConf.WaitForStateContext(ctx)
@@ -483,15 +502,13 @@ func resourceDelete(ctx context.Context, d *schema.ResourceData, meta any) diag.
 		return diag.FromErr(fmt.Errorf(errorPeersDelete, peerID, err))
 	}
 
-	log.Println("[INFO] Waiting for MongoDB Network Peering Connection to be destroyed")
-
 	stateConf := &retry.StateChangeConf{
 		Pending:    []string{"AVAILABLE", "INITIATING", "PENDING_ACCEPTANCE", "FINALIZING", "ADDING_PEER", "WAITING_FOR_USER", "TERMINATING", "DELETING"},
 		Target:     []string{"DELETED"},
 		Refresh:    resourceRefreshFunc(ctx, peerID, projectID, "", conn.NetworkPeeringApi),
-		Timeout:    1 * time.Hour,
-		MinTimeout: 30 * time.Second,
-		Delay:      10 * time.Second, // Wait 10 secs before starting
+		Timeout:    d.Timeout(schema.TimeoutDelete),
+		MinTimeout: minTimeout,
+		Delay:      minTimeout,
 	}
 
 	_, err = stateConf.WaitForStateContext(ctx)
@@ -516,19 +533,19 @@ func resourceImportState(ctx context.Context, d *schema.ResourceData, meta any)
 
 	peer, _, err := conn.NetworkPeeringApi.GetPeeringConnection(ctx, projectID, peerID).Execute()
 	if err != nil {
-		return nil, fmt.Errorf("couldn't import peer %s in project %s, error: %s", peerID, projectID, err)
+		return nil, fmt.Errorf("couldn't import peer %s in project %s, error: %w", peerID, projectID, err)
 	}
 
 	if err := d.Set("project_id", projectID); err != nil {
-		log.Printf("[WARN] Error setting project_id for (%s): %s", peerID, err)
+		return nil, fmt.Errorf("error setting project_id while importing peer %s in project %s, error: %w", peerID, projectID, err)
 	}
 
 	if err := d.Set("container_id", peer.GetContainerId()); err != nil {
-		log.Printf("[WARN] Error setting container_id for (%s): %s", peerID, err)
+		return nil, fmt.Errorf("error setting container_id while importing peer %s in project %s, error: %w", peerID, projectID, err)
 	}
 
 	if err := d.Set("provider_name", providerName); err != nil {
-		log.Printf("[WARN] Error setting provider_name for (%s): %s", peerID, err)
+		return nil, fmt.Errorf("error setting provider_name while importing peer %s in project %s, error: %w", peerID, projectID, err)
 	}
 
 	d.SetId(conversion.EncodeStateID(map[string]string{
@@ -547,9 +564,6 @@ func resourceRefreshFunc(ctx context.Context, peerID, projectID, containerID str
 			if validate.StatusNotFound(resp) {
 				return "", "DELETED", nil
 			}
-
-			log.Printf("error reading MongoDB Network Peering Connection %s: %s", peerID, err)
-
 			return nil, "", err
 		}
 
@@ -559,8 +573,6 @@ func resourceRefreshFunc(ctx context.Context, peerID, projectID, containerID str
 			status = c.GetStatusName()
 		}
 
-		log.Printf("[DEBUG] status for MongoDB Network Peering Connection: %s: %s", peerID, status)
-
 		/* We need to get the provisioned status from Mongo container that contains the peering connection
 		 * to validate if it has changed to true. This means that the reciprocal connection in Mongo side
 		 * is right, and the Mongo parameters used on the Google side to configure the reciprocal connection

internal/service/networkpeering/resource_network_peering.go

@@ -3,7 +3,6 @@ package networkpeering_test
 import (
 	"context"
 	"fmt"
-	"log"
 	"os"
 	"regexp"
 	"strings"
@@ -191,13 +190,37 @@ func TestAccNetworkRSNetworkPeering_AWSDifferentRegionName(t *testing.T) {
 		CheckDestroy:             acc.CheckDestroyNetworkPeering,
 		Steps: []resource.TestStep{
 			{
-				Config: configAWS(orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlock, containerRegion, peerRegion),
+				Config: configAWS(orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlock, containerRegion, peerRegion, false),
 				Check:  resource.ComposeAggregateTestCheckFunc(checks...),
 			},
 		},
 	})
 }
 
+func TestAccNetworkNetworkPeering_timeouts(t *testing.T) {
+	var (
+		orgID           = os.Getenv("MONGODB_ATLAS_ORG_ID")
+		vpcID           = os.Getenv("AWS_VPC_ID")
+		vpcCIDRBlock    = os.Getenv("AWS_VPC_CIDR_BLOCK")
+		awsAccountID    = os.Getenv("AWS_ACCOUNT_ID")
+		containerRegion = os.Getenv("AWS_REGION")
+		peerRegion      = conversion.MongoDBRegionToAWSRegion(containerRegion)
+		providerName    = "AWS"
+		projectName     = acc.RandomProjectName()
+	)
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { acc.PreCheckPeeringEnvAWS(t) },
+		ProtoV6ProviderFactories: acc.TestAccProviderV6Factories,
+		CheckDestroy:             acc.CheckDestroyNetworkPeering, // resource is deleted when creation times out
+		Steps: []resource.TestStep{
+			{
+				Config:      configAWS(orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlock, containerRegion, peerRegion, true),
+				ExpectError: regexp.MustCompile("will run cleanup because delete_on_create_timeout is true"),
+			},
+		},
+	})
+}
+
 func basicAWSTestCase(tb testing.TB) *resource.TestCase {
 	tb.Helper()
 	var (
@@ -213,12 +236,12 @@ func basicAWSTestCase(tb testing.TB) *resource.TestCase {
 	checks := commonChecksAWS(vpcID, providerName, awsAccountID, vpcCIDRBlock, peerRegion)
 
 	return &resource.TestCase{
-		PreCheck:                 func() { acc.PreCheckBasic(tb); acc.PreCheckPeeringEnvAWS(tb) },
+		PreCheck:                 func() { acc.PreCheckPeeringEnvAWS(tb) },
 		ProtoV6ProviderFactories: acc.TestAccProviderV6Factories,
 		CheckDestroy:             acc.CheckDestroyNetworkPeering,
 		Steps: []resource.TestStep{
 			{
-				Config: configAWS(orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlock, containerRegion, peerRegion),
+				Config: configAWS(orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlock, containerRegion, peerRegion, false),
 				Check:  resource.ComposeAggregateTestCheckFunc(checks...),
 			},
 			{
@@ -272,15 +295,25 @@ func checkExists(resourceName string) resource.TestCheckFunc {
 			return fmt.Errorf("no ID is set")
 		}
 		ids := conversion.DecodeStateID(rs.Primary.ID)
-		log.Printf("[DEBUG] projectID: %s", ids["project_id"])
 		if _, _, err := acc.ConnV2().NetworkPeeringApi.GetPeeringConnection(context.Background(), ids["project_id"], ids["peer_id"]).Execute(); err == nil {
 			return nil
 		}
 		return fmt.Errorf("peer(%s:%s) does not exist", rs.Primary.Attributes["project_id"], rs.Primary.Attributes["peer_id"])
 	}
 }
 
-func configAWS(orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlock, awsRegionContainer, awsRegionPeer string) string {
+func configAWS(orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlock, awsRegionContainer, awsRegionPeer string, forceTimeout bool) string {
+	var extraConfig string
+	if forceTimeout {
+		extraConfig = `
+			delete_on_create_timeout = true # default value
+			timeouts {
+				create = "10s"
+				update = "10s"
+				delete = "10s"
+			}
+		`
+	}
 	return fmt.Sprintf(`
 	resource "mongodbatlas_project" "my_project" {
 		name   = %[2]q
@@ -301,6 +334,7 @@ func configAWS(orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlo
 		route_table_cidr_block  = %[6]q
 		vpc_id					= %[4]q
 		aws_account_id	        = %[5]q
+		%[9]s
 	}
 
 	data "mongodbatlas_network_peering" "test" {
@@ -311,7 +345,7 @@ func configAWS(orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlo
 	data "mongodbatlas_network_peerings" "test" {
 		project_id = mongodbatlas_network_peering.test.project_id
 	}
-`, orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlock, awsRegionContainer, awsRegionPeer)
+`, orgID, projectName, providerName, vpcID, awsAccountID, vpcCIDRBlock, awsRegionContainer, awsRegionPeer, extraConfig)
 }
 
 func configAzure(projectID, providerName, directoryID, subscriptionID, resourceGroupName, vNetName string) string {