-
Notifications
You must be signed in to change notification settings - Fork 205
feat: Long-running operation improvements for mongodbatlas_encryption_at_rest_private_endpoint
resource
#3561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
cddcf57
5d6e1f9
df219b9
695283b
4344479
5858275
37c287e
72aca36
d77b328
6fe7345
90bb920
26b8dc7
8c61292
06aad46
4aa5b41
622b1a1
dcade6f
bf15a87
7547e48
c246282
d73d690
bb81f45
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
```release-note:enhancement | ||
resource/mongodbatlas_encryption_at_rest_private_endpoint: Adds `timeouts` attribute for create and delete operations | ||
``` | ||
|
||
```release-note:enhancement | ||
resource/mongodbatlas_encryption_at_rest_private_endpoint: Adds `delete_on_create_timeout` attribute to indicate whether to delete the resource if its creation times out | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ import ( | |
"github.com/hashicorp/terraform-plugin-framework/path" | ||
"github.com/hashicorp/terraform-plugin-framework/resource" | ||
|
||
"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/retrystrategy" | ||
"github.com/mongodb/terraform-provider-mongodbatlas/internal/common/validate" | ||
|
@@ -63,13 +64,28 @@ func (r *encryptionAtRestPrivateEndpointRS) Create(ctx context.Context, req reso | |
return | ||
} | ||
|
||
finalResp, err := waitStateTransition(ctx, projectID, cloudProvider, createResp.GetId(), connV2.EncryptionAtRestUsingCustomerKeyManagementApi) | ||
createTimeout := cleanup.ResolveTimeout(ctx, &earPrivateEndpointPlan.Timeouts, cleanup.OperationCreate, &resp.Diagnostics) | ||
if resp.Diagnostics.HasError() { | ||
return | ||
} | ||
|
||
finalResp, err := waitStateTransition(ctx, projectID, cloudProvider, createResp.GetId(), connV2.EncryptionAtRestUsingCustomerKeyManagementApi, createTimeout) | ||
err = cleanup.HandleCreateTimeout(cleanup.ResolveDeleteOnCreateTimeout(earPrivateEndpointPlan.DeleteOnCreateTimeout), err, func(ctxCleanup context.Context) error { | ||
cleanResp, cleanErr := connV2.EncryptionAtRestUsingCustomerKeyManagementApi.RequestEncryptionAtRestPrivateEndpointDeletion(ctxCleanup, projectID, cloudProvider, createResp.GetId()).Execute() | ||
if validate.StatusNotFound(cleanResp) { | ||
return nil | ||
} | ||
Comment on lines
+75
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q: I see in some resources (e.g. networkpeering) the sanity check of seeing if delete return 404 is not being made. For consistency, would it make sense to move this check into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case would consider that privatelinkendpointservice, privatelinkendpoint, onlinearchive, networkpeering, and cloudbackupsnapshot do not have this check in their cleanup function. Not certain if we want to keep this as a general practice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is unrelated to long-running operations, but we could create a CLOUDP to investigate and unify behavior on 404 across the resources you mentioned? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i suppose we could keep the code simple here, e.g.:
and in HandleCreateTimeout don't raise error if cleanup funcs return 404 error, if we expect this will happen with most of the resources |
||
return cleanErr | ||
}) | ||
|
||
if err != nil { | ||
resp.Diagnostics.AddError("error when waiting for status transition in creation", err.Error()) | ||
return | ||
} | ||
|
||
privateEndpointModel := NewTFEarPrivateEndpoint(*finalResp, projectID) | ||
privateEndpointModel.Timeouts = earPrivateEndpointPlan.Timeouts | ||
privateEndpointModel.DeleteOnCreateTimeout = earPrivateEndpointPlan.DeleteOnCreateTimeout | ||
resp.Diagnostics.Append(resp.State.Set(ctx, privateEndpointModel)...) | ||
|
||
diags := CheckErrorMessageAndStatus(finalResp) | ||
|
@@ -98,7 +114,10 @@ func (r *encryptionAtRestPrivateEndpointRS) Read(ctx context.Context, req resour | |
return | ||
} | ||
|
||
resp.Diagnostics.Append(resp.State.Set(ctx, NewTFEarPrivateEndpoint(*endpointModel, projectID))...) | ||
privateEndpointModel := NewTFEarPrivateEndpoint(*endpointModel, projectID) | ||
privateEndpointModel.Timeouts = earPrivateEndpointState.Timeouts | ||
privateEndpointModel.DeleteOnCreateTimeout = earPrivateEndpointState.DeleteOnCreateTimeout | ||
resp.Diagnostics.Append(resp.State.Set(ctx, privateEndpointModel)...) | ||
|
||
diags := CheckErrorMessageAndStatus(endpointModel) | ||
resp.Diagnostics.Append(diags...) | ||
|
@@ -124,7 +143,12 @@ func (r *encryptionAtRestPrivateEndpointRS) Delete(ctx context.Context, req reso | |
return | ||
} | ||
|
||
model, err := WaitDeleteStateTransition(ctx, projectID, cloudProvider, endpointID, connV2.EncryptionAtRestUsingCustomerKeyManagementApi) | ||
deleteTimeout := cleanup.ResolveTimeout(ctx, &earPrivateEndpointState.Timeouts, cleanup.OperationDelete, &resp.Diagnostics) | ||
if resp.Diagnostics.HasError() { | ||
return | ||
} | ||
|
||
model, err := WaitDeleteStateTransition(ctx, projectID, cloudProvider, endpointID, connV2.EncryptionAtRestUsingCustomerKeyManagementApi, deleteTimeout) | ||
if err != nil { | ||
resp.Diagnostics.AddError("error when waiting for status transition in delete", err.Error()) | ||
return | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import ( | |
"context" | ||
"fmt" | ||
"os" | ||
"regexp" | ||
"testing" | ||
"time" | ||
|
||
|
@@ -32,6 +33,34 @@ func TestAccEncryptionAtRestPrivateEndpoint_Azure_basic(t *testing.T) { | |
resource.Test(t, *basicTestCaseAzure(t)) | ||
} | ||
|
||
func TestAccEncryptionAtRestPrivateEndpoint_createTimeoutWithDeleteOnCreate(t *testing.T) { | ||
// This test is skipped because it creates a race condition with other tests: | ||
// 1. This test creates an encryption at rest private endpoint with a 1s timeout, causing it to fail and trigger cleanup | ||
// 2. The private endpoint deletion doesn't complete immediately | ||
// 3. Other tests share the same project and attempt to disable encryption at rest during cleanup | ||
// 4. MongoDB Atlas returns "CANNOT_DISABLE_ENCRYPTION_AT_REST_REQUIRE_PRIVATE_NETWORKING_WHILE_PRIVATE_ENDPOINTS_EXIST" | ||
// because the private endpoint from this test is still being deleted | ||
// This race condition occurs even when tests don't run in parallel due to the async nature of private endpoint deletion. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it work if using a different project? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, project needs some set up in order to make this work. |
||
acc.SkipTestForCI(t) | ||
var ( | ||
createTimeout = "1s" | ||
deleteOnCreateTimeout = true | ||
region = conversion.AWSRegionToMongoDBRegion(os.Getenv("AWS_REGION")) | ||
// Create encryption at rest configuration outside of test configuration to avoid cleanup issues | ||
projectID = acc.EncryptionAtRestExecution(t) | ||
) | ||
resource.Test(t, resource.TestCase{ | ||
PreCheck: func() { acc.PreCheckEncryptionAtRestEnvAWS(t) }, | ||
ProtoV6ProviderFactories: acc.TestAccProviderV6Factories, | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: configEARPrivateEndpointWithTimeout(projectID, region, acc.TimeoutConfig(&createTimeout, nil, nil), &deleteOnCreateTimeout), | ||
ExpectError: regexp.MustCompile("will run cleanup because delete_on_create_timeout is true"), | ||
}, | ||
}, | ||
}) | ||
} | ||
|
||
func basicTestCaseAzure(tb testing.TB) *resource.TestCase { | ||
tb.Helper() | ||
var ( | ||
|
@@ -316,19 +345,57 @@ func checkBasic(projectID, cloudProvider, region string, expectApproved bool) re | |
} | ||
|
||
func configAWSBasic(projectID string, awsKms *admin.AWSKMSConfiguration, region string) string { | ||
return configAWSBasicWithTimeout(projectID, awsKms, region, "", nil) | ||
} | ||
|
||
func configAWSBasicWithTimeout(projectID string, awsKms *admin.AWSKMSConfiguration, region, timeoutConfig string, deleteOnCreateTimeout *bool) string { | ||
encryptionAtRestConfig := acc.ConfigAwsKms(projectID, awsKms, false, true, false) | ||
|
||
deleteOnCreateTimeoutConfig := "" | ||
if deleteOnCreateTimeout != nil { | ||
deleteOnCreateTimeoutConfig = fmt.Sprintf(` | ||
delete_on_create_timeout = %[1]t | ||
`, *deleteOnCreateTimeout) | ||
} | ||
|
||
config := fmt.Sprintf(` | ||
%[1]s | ||
resource "mongodbatlas_encryption_at_rest_private_endpoint" "test" { | ||
project_id = mongodbatlas_encryption_at_rest.test.project_id | ||
cloud_provider = "AWS" | ||
region_name = %[2]q | ||
%[3]s | ||
%[4]s | ||
} | ||
%[3]s | ||
%[5]s | ||
`, encryptionAtRestConfig, region, configDS()) | ||
`, encryptionAtRestConfig, region, deleteOnCreateTimeoutConfig, timeoutConfig, configDS()) | ||
|
||
return config | ||
} | ||
|
||
func configEARPrivateEndpointWithTimeout(projectID, region, timeoutConfig string, deleteOnCreateTimeout *bool) string { | ||
deleteOnCreateTimeoutConfig := "" | ||
if deleteOnCreateTimeout != nil { | ||
deleteOnCreateTimeoutConfig = fmt.Sprintf(` | ||
delete_on_create_timeout = %[1]t | ||
`, *deleteOnCreateTimeout) | ||
} | ||
|
||
config := fmt.Sprintf(` | ||
resource "mongodbatlas_encryption_at_rest_private_endpoint" "test" { | ||
project_id = %[1]q | ||
cloud_provider = "AWS" | ||
region_name = %[2]q | ||
%[3]s | ||
%[4]s | ||
} | ||
%[5]s | ||
`, projectID, region, deleteOnCreateTimeoutConfig, timeoutConfig, configDS()) | ||
|
||
return config | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oarbusi I think we should leave a short why the update is not there, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update does not have timeout because the update operation is not supported in this resource. Considering that there's a note in the resource doc stating that update is not supported, I think it's not necessary to mention why there's no timeout for the update