-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Security group retries #9812
Security group retries #9812
Conversation
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.
Two style changes, otherwise LGTM 🚀
--- PASS: TestIpPermissionIDHash (0.00s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (4.50s)
--- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (6.55s)
--- PASS: TestAccAWSSecurityGroupRule_Egress (19.54s)
--- PASS: TestAccAWSSecurityGroupRule_Issue5310 (20.10s)
--- PASS: TestProtocolStateFunc (0.00s)
--- PASS: TestProtocolForValue (0.00s)
--- PASS: TestResourceAwsSecurityGroupExpandCollapseRules (0.00s)
--- PASS: TestResourceAwsSecurityGroupIPPermGather (0.00s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription (20.45s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (27.54s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (28.16s)
--- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (28.03s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (28.76s)
--- PASS: TestAccAWSSecurityGroupRule_MultiIngress (30.07s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription (30.33s)
--- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (35.06s)
--- PASS: TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash (30.01s)
--- PASS: TestAccAWSSecurityGroupRule_SelfSource (46.79s)
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts_NonZeroPorts (44.22s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (54.82s)
--- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (54.30s)
--- PASS: TestAccAWSSecurityGroupRule_SelfReference (55.38s)
--- PASS: TestAccAWSSecurityGroup_importIpv6 (36.24s)
--- PASS: TestAccAWSSecurityGroup_importBasic (39.60s)
--- PASS: TestAccAWSSecurityGroup_basic (32.27s)
--- PASS: TestAccAWSSecurityGroup_namePrefix (13.63s)
--- PASS: TestAccAWSSecurityGroup_importSourceSecurityGroup (42.30s)
--- PASS: TestAccAWSSecurityGroup_importIPRangesWithSameRules (44.30s)
--- PASS: TestAccAWSSecurityGroup_ipv6 (19.19s)
--- PASS: TestAccAWSSecurityGroup_vpcProtoNumIngress (22.70s)
--- PASS: TestAccAWSSecurityGroup_vpcNegOneIngress (29.29s)
--- PASS: TestAccAWSSecurityGroup_self (42.91s)
--- PASS: TestAccAWSSecurityGroup_Egress_ConfigMode (65.41s)
--- SKIP: TestAccAWSSecurityGroup_DefaultEgress_Classic (1.12s)
--- PASS: TestAccAWSSecurityGroup_importPrefixList (73.11s)
--- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (103.52s)
--- PASS: TestAccAWSSecurityGroup_MultiIngress (34.03s)
--- PASS: TestAccAWSSecurityGroup_invalidCIDRBlock (1.98s)
--- PASS: TestAccAWSSecurityGroupRule_MultiDescription (90.65s)
--- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (110.34s)
--- PASS: TestAccAWSSecurityGroup_ruleGathering (63.73s)
--- SKIP: TestAccAWSSecurityGroup_ingressWithCidrAndSGs_classic (1.07s)
--- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (112.66s)
--- PASS: TestAccAWSSecurityGroup_generatedName (25.60s)
--- PASS: TestAccAWSSecurityGroup_drift (24.95s)
--- PASS: TestAccAWSSecurityGroup_DefaultEgress_VPC (33.87s)
--- PASS: TestAccAWSSecurityGroup_drift_complex (30.01s)
--- PASS: TestAccAWSSecurityGroup_Change (64.62s)
--- PASS: TestAccAWSSecurityGroup_importIPRangeAndSecurityGroupWithSameRules (109.72s)
--- PASS: TestAccAWSSecurityGroup_CIDRandGroups (37.33s)
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGs (38.63s)
--- PASS: TestAccAWSSecurityGroup_importSelf (119.81s)
--- PASS: TestAccAWSSecurityGroup_ipv4andipv6Egress (39.14s)
--- PASS: TestAccAWSSecurityGroup_Ingress_ConfigMode (123.24s)
--- PASS: TestAccAWSSecurityGroup_ingressWithPrefixList (52.98s)
--- PASS: TestAccAWSSecurityGroup_failWithDiffMismatch (52.89s)
--- PASS: TestAccAWSSecurityGroup_tags (63.39s)
--- PASS: TestAccAWSSecurityGroup_RuleDescription (93.59s)
--- PASS: TestAccAWSSecurityGroup_egressWithPrefixList (65.30s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededAppend (60.20s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitCidrBlockExceededAppend (54.50s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededPrepend (57.71s)
--- PASS: TestAccAWSSecurityGroup_rulesDropOnError (48.32s)
--- PASS: TestAccAWSSecurityGroup_vpc (128.12s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededAllNew (57.76s)
--- PASS: TestAccAWSSecurityGroupRule_Race (200.76s)
--- PASS: TestAccAWSSecurityGroup_forceRevokeRules_false (1252.95s)
--- PASS: TestAccAWSSecurityGroup_forceRevokeRules_true (1293.55s)
Co-Authored-By: Brian Flad <bflad417@gmail.com>
Co-Authored-By: Brian Flad <bflad417@gmail.com>
This has been released in version 2.25.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
…yableError Reference: hashicorp/terraform#17220 Reference: hashicorp/terraform-provider-aws#9779 (comment) Reference: hashicorp/terraform-provider-aws#9812 (comment) It is currently possible to introduce subtle bugs or crashes when using `resource.RetryableError` and `resource.NonRetryableError` and allowing a `nil` error input. This PR proposes behavior that requires providers to be explicit with their usage of these errors, otherwise returns a bug reporting message to the operator. For example (https://github.com/terraform-providers/terraform-provider-aws/blob/4c0387645f982ddcd51b3ffe2cc8992c06fb9c2c/aws/resource_aws_elastic_beanstalk_application.go#L105): ```go var app *elasticbeanstalk.ApplicationDescription err := resource.Retry(30*time.Second, func() *resource.RetryError { var err error app, err = getBeanstalkApplication(d.Id(), conn) if err != nil { return resource.NonRetryableError(err) } if app == nil { if d.IsNewResource() { return resource.RetryableError(fmt.Errorf("Elastic Beanstalk Application %q not found", d.Id())) } // err is nil here return resource.NonRetryableError(err) } return nil }) if err != nil { return err } // app can be nil here, so this can crash Terraform d.Set("name", app.ApplicationName) ``` Another example (https://github.com/terraform-providers/terraform-provider-alicloud/blob/927a8e82386e0b32718aa5eef254255fdc36b070/alicloud/resource_alicloud_disk_attachment.go#L112): ```go return resource.Retry(5*time.Minute, func() *resource.RetryError { err := conn.DetachDisk(instanceID, diskID) if err != nil { if IsExceptedError(err, DiskIncorrectStatus) || IsExceptedError(err, InstanceLockedForSecurity) || IsExceptedError(err, DiskInvalidOperation) { return resource.RetryableError(fmt.Errorf("Detach Disk timeout and got an error: %#v", err)) } } disks, _, descErr := conn.DescribeDisks(&ecs.DescribeDisksArgs{ RegionId: getRegion(d, meta), DiskIds: []string{diskID}, }) if descErr != nil { log.Printf("[ERROR] Disk %s is not detached.", diskID) // err can be nil here return resource.NonRetryableError(err) } for _, disk := range disks { if disk.Status != ecs.DiskStatusAvailable { return resource.RetryableError(fmt.Errorf("Detach Disk timeout and got an error: %#v", err)) } } return nil }) ``` Another example (https://github.com/terraform-providers/terraform-provider-aws/blob/75c32b375140813b7994f8031e74b8588a08035a/aws/resource_aws_kms_alias.go#L176-L190): ```go func retryFindKmsAliasByName(conn *kms.KMS, name string) (*kms.AliasListEntry, error) { var resp *kms.AliasListEntry err := resource.Retry(1*time.Minute, func() *resource.RetryError { var err error resp, err = findKmsAliasByName(conn, name, nil) if err != nil { return resource.NonRetryableError(err) } if resp == nil { // err is nil here, so returns nil return resource.RetryableError(err) } return nil }) return resp, err } ``` Another example (https://github.com/terraform-providers/terraform-provider-aws/blob/00909998d919faf5494ab8f6b38241deb1957d99/aws/resource_aws_internet_gateway.go#L55-L65): ```go err = resource.Retry(5*time.Minute, func() *resource.RetryError { igRaw, _, err := IGStateRefreshFunc(conn, d.Id())() if igRaw != nil { return nil } if err == nil { // err is always nil here, so it will never retry return resource.RetryableError(err) } else { return resource.NonRetryableError(err) } }) ```
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Community Note
Relates #7873
Release note for CHANGELOG:
Output from acceptance testing: