Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor: use retry.StateChangeConf for encryption-at-rest resource. #1463

Closed
wants to merge 1 commit into from
Closed

refactor: use retry.StateChangeConf for encryption-at-rest resource. #1463

wants to merge 1 commit into from

Conversation

marcosuma
Copy link
Collaborator

@marcosuma marcosuma commented Sep 11, 2023 •
edited
Loading

Description

Use retry.StateChangeConf instead of the for loop to remove technical debt and improve retry strategy.

Link to any related issue(s): https://jira.mongodb.org/browse/INTMDB-1039

Type of change:

  • Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
  • New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have signed the MongoDB CLA
  • I have read the contribution guidelines
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Further comments

@marcosuma marcosuma requested a review from a team as a code owner September 11, 2023 09:51
@marcosuma
Copy link
Collaborator Author

Running terraform apply on the example terraform-provider-mongodbatlas/examples/test-upgrade/encryption-at-rest/aws/v101:

mongodbatlas_project.test: Refreshing state... [id=64fecfd3da30680cf9043172]
mongodbatlas_cloud_provider_access_setup.setup_only: Refreshing state... [id=aWQ=:NjRmZWNmZDU5MWRhNjY2Nzg2OTRkNGI2-cHJvamVjdF9pZA==:NjRmZWNmZDNkYTMwNjgwY2Y5MDQzMTcy-cHJvdmlkZXJfbmFtZQ==:QVdT]
aws_iam_role.test_role: Refreshing state... [id=mongo_setup_test_role]
aws_iam_role_policy.test_policy: Refreshing state... [id=mongo_setup_test_role:mongo_setup_policy]
mongodbatlas_cloud_provider_access_authorization.auth_role: Refreshing state... [id=aWQ=:NjRmZWNmZDU5MWRhNjY2Nzg2OTRkNGI2-cHJvamVjdF9pZA==:NjRmZWNmZDNkYTMwNjgwY2Y5MDQzMTcy]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # mongodbatlas_encryption_at_rest.test will be created
  + resource "mongodbatlas_encryption_at_rest" "test" {
      + id         = (known after apply)
      + project_id = "64fecfd3da30680cf9043172"

      + aws_kms_config {
          + customer_master_key_id = (sensitive value)
          + enabled                = true
          + region                 = "US_EAST_1"
          + role_id                = "64fecfd591da66678694d4b6"
        }
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

mongodbatlas_encryption_at_rest.test: Creating...
mongodbatlas_encryption_at_rest.test: Creation complete after 1s [id=64fecfd3da30680cf9043172]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Outputs:

project_id = "64fecfd3da30680cf9043172"
role_name = "mongo_setup_test_role"
role_policy_name = "mongo_setup_policy"

andreaangiolillo
andreaangiolillo reviewed Sep 11, 2023
View reviewed changes
Copy link
Collaborator

@andreaangiolillo andreaangiolillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments. Make sure to run the acceptance tests locally

mongodbatlas/resource_mongodbatlas_encryption_at_rest.go
Comment on lines +245 to +246
Pending: []string{"RETRY"},
Target: []string{"COMPLETED", "ERROR"},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Should we make RETRY/COMPLETE/ERROR constants and use them in all the resources with a similar strategy?

mongodbatlas/resource_mongodbatlas_encryption_at_rest.go
Comment on lines +253 to +256
_, err := stateConf.WaitForStateContext(ctx)
if err != nil {
return diag.FromErr(fmt.Errorf(errorCreateEncryptionAtRest, err))
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_, err := stateConf.WaitForStateContext(ctx)
if err != nil {
return diag.FromErr(fmt.Errorf(errorCreateEncryptionAtRest, err))
}
if _, err := stateConf.WaitForStateContext(ctx); err != nil {
return diag.FromErr(fmt.Errorf(errorCreateEncryptionAtRest, err))
}

mongodbatlas/resource_mongodbatlas_encryption_at_rest.go
Refresh: resourceMongoDBAtlasEncryptionAtRestCreateRefreshFunc(ctx, d.Get("project_id").(string), conn, encryptionAtRestReq),
Timeout: timeout,
MinTimeout: 30 * time.Second,
Delay: 1 * time.Minute,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the delay was 10 seconds. Any reason why is now 1 minutes?

mongodbatlas/resource_mongodbatlas_encryption_at_rest.go
Comment on lines 266 to 269
if strings.Contains(err.Error(), "CANNOT_ASSUME_ROLE") || strings.Contains(err.Error(), "INVALID_AWS_CREDENTIALS") ||
strings.Contains(err.Error(), "CLOUD_PROVIDER_ACCESS_ROLE_NOT_AUTHORIZED") {
log.Printf("warning issue performing authorize EncryptionsAtRest not done try again: %s \n", err.Error())
log.Println("retrying ")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use a more golang idiomatic way to handle the error: with error.is() https://gosamples.dev/check-error-type/

@marcosuma marcosuma closed this Sep 19, 2023
@marcosuma marcosuma deleted the INTMDB-1039-improve-retry-strategy branch September 19, 2023 09:25
@marcosuma marcosuma mentioned this pull request Sep 19, 2023
Merged
10 tasks
marcosuma added a commit that referenced this pull request Sep 20, 2023
@marcosuma
Addresses comments in previous PR: #1463.
4fb54b1
marcosuma added a commit that referenced this pull request Sep 26, 2023
@marcosuma
refactor: use retry.StateChangeConf for encryption-at-rest resource. (#…
1fabeeb 
…1477)

* refactor: use retry.StateChangeConf for encryption-at-rest resource.

* Addresses comments in previous PR: #1463.

* improves error checking in the if statement.

* moves retry states separately to be reused.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreaangiolillo andreaangiolillo andreaangiolillo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@marcosuma @andreaangiolillo