-
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
Conversation
@@ -48,6 +48,7 @@ func TestAccFlexClusterRS_createTimeoutWithDeleteOnCreateFlex(t *testing.T) { | |||
} | |||
|
|||
func TestAccFlexClusterRS_updateDeleteTimeout(t *testing.T) { | |||
acc.SkipTestForCI(t) // Update is consistently too fast and it does not time out, making the test flaky |
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.
didn't notice this until now, but it does fail quite consistently due to the step 2 of the test not having the expected failure because it does not time out on the update
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.
can modifying the region in the second make the update operation longer and ensure 1s will always timeout?
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.
region is not updatable, the cluster would need to be recreated to change the region
encryption tests failing due to back clean up |
APIx bot: a message has been sent to Docs Slack channel |
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.
Pull Request Overview
This PR implements long-running operation improvements for the mongodbatlas_encryption_at_rest_private_endpoint
resource by adding proper timeout support and delete_on_create_timeout
functionality. These changes align with the provider's pattern for handling long-running operations and provide better control over resource creation timeouts.
- Added
timeouts
block support for Create and Delete operations - Added
delete_on_create_timeout
attribute (defaults to true) for automatic cleanup on creation timeout - Updated state transition functions to accept timeout parameters
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
internal/service/encryptionatrestprivateendpoint/resource_schema.go | Added timeouts and delete_on_create_timeout attributes to resource schema |
internal/service/encryptionatrestprivateendpoint/resource.go | Integrated timeout handling and cleanup logic into Create/Delete operations |
internal/service/encryptionatrestprivateendpoint/state_transition.go | Updated state transition functions to accept timeout parameters |
internal/service/encryptionatrestprivateendpoint/model.go | Added separate data source model type and helper function |
internal/service/encryptionatrestprivateendpoint/data_source_schema.go | Created separate data source model without timeout fields |
internal/service/encryptionatrestprivateendpoint/resource_test.go | Added timeout test and cleanup handling |
internal/testutil/acc/encryption_at_rest.go | Added encryption at rest test utility functions |
internal/testutil/acc/shared_resource.go | Added encryption at rest cleanup support |
internal/service/flexcluster/resource.go | Moved common timeout resolution function to shared cleanup package |
docs/resources/encryption_at_rest_private_endpoint.md | Updated documentation with new timeout attributes |
Co-authored-by: Copilot <[email protected]>
### Read-Only | ||
|
||
- `error_message` (String) Error message for failures associated with the Encryption At Rest private endpoint. | ||
- `id` (String) Unique 24-hexadecimal digit string that identifies the Private Endpoint Service. | ||
- `private_endpoint_connection_name` (String) Connection name of the Azure Private Endpoint. | ||
- `status` (String) State of the Encryption At Rest private endpoint. | ||
|
||
<a id="nestedatt--timeouts"></a> | ||
### Nested Schema for `timeouts` |
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
if validate.StatusNotFound(cleanResp) { | ||
return nil | ||
} |
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.
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 cleanup.HandleCreateTimeout
, and have cleanup
only call the DELETE operation?
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.
IMO HandleCreateTimeout
should only handle the delete on create logic, and avoid doing the 404 case, to keep the method focused and clear
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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
i suppose we could keep the code simple here, e.g.:
err = cleanup.HandleCreateTimeout(cleanup.ResolveDeleteOnCreateTimeout(earPrivateEndpointPlan.DeleteOnCreateTimeout), err, func(ctxCleanup context.Context) error {
_, cleanErr := connV2.EncryptionAtRestUsingCustomerKeyManagementApi.RequestEncryptionAtRestPrivateEndpointDeletion(ctxCleanup, projectID, cloudProvider, createResp.GetId()).Execute()
return cleanErr
})
and in HandleCreateTimeout don't raise error if cleanup funcs return 404 error, if we expect this will happen with most of the resources
// EncryptionAtRestExecution creates an encryption at rest configuration for test execution. | ||
func EncryptionAtRestExecution(tb testing.TB) string { |
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.
can be good to clarify it uses a fixed project and what env variables are leveraged. Potentially passing values as parameter to the function also makes it more explicit.
// 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
no, project needs some set up in order to make this work.
Description
Implements long-running operation improvements for
mongodbatlas_encryption_at_rest_private_endpoint
resource, adding proper timeout support anddelete_on_create_timeout
functionality.Changes
Added timeout support: mongodbatlas_encryption_at_rest_private_endpoint Create and Delete operations now respect configured timeouts block
Added
delete_on_create_timeout
: New optional field (defaults to true) that automatically deletes cluster if creation times outAdded timeout testing: New acceptance test validates timeout and cleanup functionality
Link to any related issue(s): CLOUDP-334357
Type of change:
Required Checklist:
Further comments