Skip to content

Commit

Permalink
provider: Migrate to iamwaiter.PropagationTimeout constant and begin …
Browse files Browse the repository at this point in the history
…enabling go-mnd linter (#17811)

Reference: #13199
Reference: #16752
Reference: #16753

IAM eventual consistency handling has long been the source of needing retries in resource logic. Due to the lack of a consistent implementation (e.g. static constant) for how long to retry for these types of errors, there have been varying retry durations. The `iamwaiter.PropagationTimeout` constant was introduced for this purpose.

This change begins by introducing the `go-mnd` linter to enforce the usage of constants in function arguments. Example reports below. The rest of the changes are the minimum required to ensure `iamwaiter.PropagationTimeout` with its 2 minute duration is applied. You will note that this is fixing the duration in some cases to slightly increase it to the standard value. Any higher durations are ignored to reduce changes for now. As such, this can be reviewed by validating that a lower duration was not introduced and skipping acceptance testing since no logic changes should be introduced.

One caveat to `go-mnd` is that it currently ignores `1` as a magic number, which is possible in usage such as `1*time.Minute`, and that ignored number cannot be overriden. An upstream issue will be created to ask the `ignore-number` configuration to overwrite instead of append.

Example previous report:

```
aws/resource_aws_api_gateway_account.go:99:23: mnd: Magic number: 2, in <argument> detected (gomnd)
	err = resource.Retry(2*time.Minute, func() *resource.RetryError {
	                     ^
```
  • Loading branch information
bflad committed Mar 26, 2021
1 parent 33560da commit f17c81e
Show file tree
Hide file tree
Showing 38 changed files with 217 additions and 65 deletions.
127 changes: 127 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ linters:
- deadcode
- errcheck
- gofmt
- gomnd
- gosimple
- ineffassign
- makezero
Expand All @@ -34,6 +35,132 @@ linters:
linters-settings:
errcheck:
ignore: github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema:ForceNew|Set,fmt:.*,io:Close
gomnd:
settings:
mnd:
checks:
- argument
ignored-files:
# Needing constants, comment ignores, switching to customizable timeouts, or retries moved to aws/config.go
- awserr.go
- data_source_aws_cognito_user_pools.go
- data_source_aws_lakeformation_permissions.go
- resource_aws_api_gateway_base_path_mapping.go
- resource_aws_appautoscaling_policy.go
- resource_aws_appautoscaling_scheduled_action.go
- resource_aws_appautoscaling_target.go
- resource_aws_autoscaling_lifecycle_hook.go
- resource_aws_backup_plan.go
- resource_aws_cloud9_environment_ec2.go
- resource_aws_cloudfront_distribution.go
- resource_aws_cloudhsm2_cluster.go
- resource_aws_cloudhsm2_hsm.go
- resource_aws_cloudwatch_event_target.go
- resource_aws_cloudwatch_log_destination.go
- resource_aws_cloudwatch_log_stream.go
- resource_aws_cloudwatch_log_subscription_filter.go
- resource_aws_codebuild_project.go
- resource_aws_codedeploy_deployment_group.go
- resource_aws_codepipeline_webhook.go
- resource_aws_config_config_rule.go
- resource_aws_config_delivery_channel.go
- resource_aws_customer_gateway.ogresource_aws_elasticsearch_domain
- resource_aws_datapipeline_pipeline.go
- resource_aws_db_instance.go
- resource_aws_db_parameter_group.go
- resource_aws_dms_endpoint.go
- resource_aws_docdb_cluster.go
- resource_aws_docdb_cluster_parameter_group.go
- resource_aws_docdb_subnet_group.go
- resource_aws_dynamodb_table.go
- resource_aws_ebs_snapshot_copy.go
- resource_aws_ebs_volume.go
- resource_aws_ec2_transit_gateway.go
- resource_aws_ecs_cluster.go
- resource_aws_ecs_service.go
- resource_aws_efs_file_system.go
- resource_aws_efs_mount_target.go
- resource_aws_elastic_beanstalk_application.go
- resource_aws_elasticache_cluster.go
- resource_aws_elasticache_parameter_group.go
- resource_aws_elasticache_replication_group.go
- resource_aws_elasticache_security_group.go
- resource_aws_elasticache_subnet_group.go
- resource_aws_elasticsearch_domain.go
- resource_aws_elasticsearch_domain_policy.go
- resource_aws_elb.go
- resource_aws_elb_attachment.go
- resource_aws_gamelift_build.go
- resource_aws_gamelift_fleet.go
- resource_aws_glue_dev_endpoint.go
- resource_aws_iam_access_key.go
- resource_aws_iam_server_certificate.go
- resource_aws_inspector_assessment_target.go
- resource_aws_instance.go
- resource_aws_internet_gateway.go
- resource_aws_iot_thing_type.go
- resource_aws_kms_external_key.go
- resource_aws_kms_grant.go
- resource_aws_kms_key.go
- resource_aws_lakeformation_data_lake_settings.go
- resource_aws_lakeformation_permissions.go
- resource_aws_lambda_event_source_mapping.go
- resource_aws_lambda_function_event_invoke_config.go
- resource_aws_lambda_permission.go
- resource_aws_lb_listener.go
- resource_aws_lb_listener_rule.go
- resource_aws_lb_target_group_attachment.go
- resource_aws_media_package_channel.go
- resource_aws_media_store_container.go
- resource_aws_msk_cluster.go
- resource_aws_neptune_cluster.go
- resource_aws_neptune_parameter_group.go
- resource_aws_network_acl.go
- resource_aws_network_acl_rule.go
- resource_aws_opsworks_stack.go
- resource_aws_organizations_account.go
- resource_aws_organizations_organizational_unit.go
- resource_aws_organizations_policy.go
- resource_aws_organizations_policy_attachment.go
- resource_aws_qldb_ledger.go
- resource_aws_ram_resource_share_accepter.go
- resource_aws_rds_cluster.go
- resource_aws_rds_cluster_parameter_group.go
- resource_aws_redshift_cluster.go
- resource_aws_redshift_snapshot_copy_grant.go
- resource_aws_redshift_snapshot_schedule.go
- resource_aws_redshift_snapshot_schedule_association.go
- resource_aws_route_table.go
- resource_aws_route_table_association.go
- resource_aws_s3_bucket.go
- resource_aws_sagemaker_model.go
- resource_aws_sagemaker_notebook_instance.go
- resource_aws_security_group_rule.go
- resource_aws_sfn_state_machine.go
- resource_aws_sqs_queue.go
- resource_aws_storagegateway_cached_iscsi_volume.go
- resource_aws_storagegateway_stored_iscsi_volume.go
- resource_aws_transfer_server.go
- resource_aws_vpc.go
- resource_aws_vpc_dhcp_options.go
- resource_aws_vpc_peering_connection_options.go
- resource_aws_vpn_gateway.go
- resource_aws_wafregional_web_acl_association.go
- resource_aws_wafv2_ip_set.go
- resource_aws_wafv2_regex_pattern_set.go
- resource_aws_wafv2_rule_group.go
- resource_aws_wafv2_web_acl.go
- tls.go
- waf_token_handlers.go
- wafregional_token_handlers.go
ignored-functions:
# AWS Go SDK
- aws.Int64
- request.ConstantWaiterDelay
- request.WithWaiterMaxAttempts
# Terraform Plugin SDK
- schema.DefaultTimeout
- validation.*

run:
timeout: 10m
4 changes: 2 additions & 2 deletions aws/resource_aws_api_gateway_account.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ package aws
import (
"fmt"
"log"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/apigateway"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

func resourceAwsApiGatewayAccount() *schema.Resource {
Expand Down Expand Up @@ -96,7 +96,7 @@ func resourceAwsApiGatewayAccountUpdate(d *schema.ResourceData, meta interface{}
otherErrMsg := "API Gateway could not successfully write to CloudWatch Logs using the ARN specified"
var out *apigateway.Account
var err error
err = resource.Retry(2*time.Minute, func() *resource.RetryError {
err = resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
out, err = conn.UpdateAccount(&input)

if err != nil {
Expand Down
7 changes: 4 additions & 3 deletions aws/resource_aws_appautoscaling_policy.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

func resourceAwsAppautoscalingPolicy() *schema.Resource {
Expand Down Expand Up @@ -209,7 +210,7 @@ func resourceAwsAppautoscalingPolicyCreate(d *schema.ResourceData, meta interfac

log.Printf("[DEBUG] ApplicationAutoScaling PutScalingPolicy: %#v", params)
var resp *applicationautoscaling.PutScalingPolicyOutput
err = resource.Retry(2*time.Minute, func() *resource.RetryError {
err = resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
var err error
resp, err = conn.PutScalingPolicy(&params)
if err != nil {
Expand Down Expand Up @@ -301,7 +302,7 @@ func resourceAwsAppautoscalingPolicyUpdate(d *schema.ResourceData, meta interfac
}

log.Printf("[DEBUG] Application Autoscaling Update Scaling Policy: %#v", params)
err := resource.Retry(2*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
_, err := conn.PutScalingPolicy(&params)
if err != nil {
if isAWSErr(err, applicationautoscaling.ErrCodeFailedResourceAccessException, "") {
Expand Down Expand Up @@ -341,7 +342,7 @@ func resourceAwsAppautoscalingPolicyDelete(d *schema.ResourceData, meta interfac
ServiceNamespace: aws.String(d.Get("service_namespace").(string)),
}
log.Printf("[DEBUG] Deleting Application AutoScaling Policy opts: %#v", params)
err = resource.Retry(2*time.Minute, func() *resource.RetryError {
err = resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
_, err = conn.DeleteScalingPolicy(&params)

if isAWSErr(err, applicationautoscaling.ErrCodeFailedResourceAccessException, "") {
Expand Down
3 changes: 2 additions & 1 deletion aws/resource_aws_appautoscaling_target.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/aws/aws-sdk-go/service/applicationautoscaling"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

func resourceAwsAppautoscalingTarget() *schema.Resource {
Expand Down Expand Up @@ -72,7 +73,7 @@ func resourceAwsAppautoscalingTargetPut(d *schema.ResourceData, meta interface{}

log.Printf("[DEBUG] Application autoscaling target create configuration %s", targetOpts)
var err error
err = resource.Retry(1*time.Minute, func() *resource.RetryError {
err = resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
_, err = conn.RegisterScalableTarget(&targetOpts)

if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion aws/resource_aws_autoscaling_group.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/terraform-providers/terraform-provider-aws/aws/internal/hashcode"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/autoscaling/waiter"
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

const (
Expand Down Expand Up @@ -735,7 +736,7 @@ func resourceAwsAutoscalingGroupCreate(d *schema.ResourceData, meta interface{})
log.Printf("[DEBUG] Auto Scaling Group create configuration: %#v", createOpts)

// Retry for IAM eventual consistency
err := resource.Retry(1*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
_, err := conn.CreateAutoScalingGroup(&createOpts)

// ValidationError: You must use a valid fully-formed launch template. Value (tf-acc-test-6643732652421074386) for parameter iamInstanceProfile.name is invalid. Invalid IAM Instance Profile name
Expand Down
4 changes: 2 additions & 2 deletions aws/resource_aws_backup_selection.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ import (
"log"
"regexp"
"strings"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/backup"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

func resourceAwsBackupSelection() *schema.Resource {
Expand Down Expand Up @@ -98,7 +98,7 @@ func resourceAwsBackupSelectionCreate(d *schema.ResourceData, meta interface{})

// Retry for IAM eventual consistency
var output *backup.CreateBackupSelectionOutput
err := resource.Retry(1*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
var err error
output, err = conn.CreateBackupSelection(input)

Expand Down
3 changes: 2 additions & 1 deletion aws/resource_aws_cloud9_environment_ec2.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

func resourceAwsCloud9EnvironmentEc2() *schema.Resource {
Expand Down Expand Up @@ -91,7 +92,7 @@ func resourceAwsCloud9EnvironmentEc2Create(d *schema.ResourceData, meta interfac
}

var out *cloud9.CreateEnvironmentEC2Output
err := resource.Retry(1*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
var err error
out, err = conn.CreateEnvironmentEC2(params)
if err != nil {
Expand Down
6 changes: 3 additions & 3 deletions aws/resource_aws_cloudtrail.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ package aws
import (
"fmt"
"log"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/cloudtrail"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

func resourceAwsCloudTrail() *schema.Resource {
Expand Down Expand Up @@ -192,7 +192,7 @@ func resourceAwsCloudTrailCreate(d *schema.ResourceData, meta interface{}) error
}

var t *cloudtrail.CreateTrailOutput
err := resource.Retry(1*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
var err error
t, err = conn.CreateTrail(&input)
if err != nil {
Expand Down Expand Up @@ -377,7 +377,7 @@ func resourceAwsCloudTrailUpdate(d *schema.ResourceData, meta interface{}) error

log.Printf("[DEBUG] Updating CloudTrail: %s", input)
var t *cloudtrail.UpdateTrailOutput
err := resource.Retry(1*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
var err error
t, err = conn.UpdateTrail(&input)
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion aws/resource_aws_codebuild_project.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/hashcode"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

func resourceAwsCodeBuildProject() *schema.Resource {
Expand Down Expand Up @@ -1232,7 +1233,7 @@ func resourceAwsCodeBuildProjectUpdate(d *schema.ResourceData, meta interface{})
params.Tags = keyvaluetags.New(d.Get("tags").(map[string]interface{})).IgnoreAws().CodebuildTags()

// Handle IAM eventual consistency
err := resource.Retry(1*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
var err error

_, err = conn.UpdateProject(params)
Expand Down
9 changes: 5 additions & 4 deletions aws/resource_aws_cognito_user_pool.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

func resourceAwsCognitoUserPool() *schema.Resource {
Expand Down Expand Up @@ -692,7 +693,7 @@ func resourceAwsCognitoUserPoolCreate(d *schema.ResourceData, meta interface{})
// IAM roles & policies can take some time to propagate and be attached
// to the User Pool
var resp *cognitoidentityprovider.CreateUserPoolOutput
err := resource.Retry(2*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
var err error
resp, err = conn.CreateUserPool(params)
if isAWSErr(err, cognitoidentityprovider.ErrCodeInvalidSmsRoleTrustRelationshipException, "Role does not have a trust relationship allowing Cognito to assume the role") {
Expand Down Expand Up @@ -735,7 +736,7 @@ func resourceAwsCognitoUserPoolCreate(d *schema.ResourceData, meta interface{})
}

// IAM Roles and Policies can take some time to propagate
err := resource.Retry(2*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
_, err := conn.SetUserPoolMfaConfig(input)

if isAWSErr(err, cognitoidentityprovider.ErrCodeInvalidSmsRoleTrustRelationshipException, "Role does not have a trust relationship allowing Cognito to assume the role") {
Expand Down Expand Up @@ -923,7 +924,7 @@ func resourceAwsCognitoUserPoolUpdate(d *schema.ResourceData, meta interface{})
}

// IAM Roles and Policies can take some time to propagate
err := resource.Retry(2*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
_, err := conn.SetUserPoolMfaConfig(input)

if isAWSErr(err, cognitoidentityprovider.ErrCodeInvalidSmsRoleTrustRelationshipException, "Role does not have a trust relationship allowing Cognito to assume the role") {
Expand Down Expand Up @@ -1124,7 +1125,7 @@ func resourceAwsCognitoUserPoolUpdate(d *schema.ResourceData, meta interface{})

// IAM roles & policies can take some time to propagate and be attached
// to the User Pool.
err := resource.Retry(2*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
var err error
_, err = conn.UpdateUserPool(params)
if isAWSErr(err, cognitoidentityprovider.ErrCodeInvalidSmsRoleTrustRelationshipException, "Role does not have a trust relationship allowing Cognito to assume the role") {
Expand Down
3 changes: 2 additions & 1 deletion aws/resource_aws_config_config_rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/hashcode"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter"
)

func resourceAwsConfigConfigRule() *schema.Resource {
Expand Down Expand Up @@ -166,7 +167,7 @@ func resourceAwsConfigConfigRulePut(d *schema.ResourceData, meta interface{}) er
Tags: keyvaluetags.New(d.Get("tags").(map[string]interface{})).IgnoreAws().ConfigserviceTags(),
}
log.Printf("[DEBUG] Creating AWSConfig config rule: %s", input)
err := resource.Retry(2*time.Minute, func() *resource.RetryError {
err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError {
_, err := conn.PutConfigRule(&input)
if err != nil {
if awsErr, ok := err.(awserr.Error); ok {
Expand Down
Loading

0 comments on commit f17c81e

Please sign in to comment.