From e7674c3ca539177283207be1330d6888ce1ba901 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Thu, 25 Mar 2021 20:00:41 -0700 Subject: [PATCH] Replace calls to `DescribeSecurityGroups` with single result with `finder.SecurityGroup...()` --- ...esource_aws_default_security_group_test.go | 31 ++-- aws/resource_aws_emr_cluster_test.go | 68 +++----- ...s_globalaccelerator_endpoint_group_test.go | 23 +-- aws/resource_aws_security_group.go | 152 ++++-------------- aws/resource_aws_security_group_rule.go | 98 +++-------- aws/resource_aws_security_group_rule_test.go | 54 ++----- aws/resource_aws_security_group_test.go | 150 ++++++----------- 7 files changed, 160 insertions(+), 416 deletions(-) diff --git a/aws/resource_aws_default_security_group_test.go b/aws/resource_aws_default_security_group_test.go index 40117d58d9e..fa65ba56f4e 100644 --- a/aws/resource_aws_default_security_group_test.go +++ b/aws/resource_aws_default_security_group_test.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/finder" ) func TestAccAWSDefaultSecurityGroup_Vpc_basic(t *testing.T) { @@ -181,24 +182,19 @@ func testAccCheckAWSDefaultSecurityGroupExists(n string, group *ec2.SecurityGrou } if rs.Primary.ID == "" { - return fmt.Errorf("No Security Group is set") + return fmt.Errorf("No EC2 Default Security Group ID is set") } conn := testAccProvider.Meta().(*AWSClient).ec2conn - req := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(rs.Primary.ID)}, - } - resp, err := conn.DescribeSecurityGroups(req) + + sg, err := finder.SecurityGroupByID(conn, rs.Primary.ID) if err != nil { return err } - if len(resp.SecurityGroups) > 0 && *resp.SecurityGroups[0].GroupId == rs.Primary.ID { - *group = *resp.SecurityGroups[0] - return nil - } + *group = *sg - return fmt.Errorf("Security Group not found") + return nil } } @@ -215,21 +211,12 @@ func testAccCheckAWSDefaultSecurityGroupEc2ClassicExists(n string, group *ec2.Se conn := testAccProviderEc2Classic.Meta().(*AWSClient).ec2conn - input := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(rs.Primary.ID)}, - } - - resp, err := conn.DescribeSecurityGroups(input) - + sg, err := finder.SecurityGroupByID(conn, rs.Primary.ID) if err != nil { - return fmt.Errorf("error describing EC2 Default Security Group (%s): %w", rs.Primary.ID, err) - } - - if len(resp.SecurityGroups) == 0 || aws.StringValue(resp.SecurityGroups[0].GroupId) != rs.Primary.ID { - return fmt.Errorf("EC2 Default Security Group (%s) not found", rs.Primary.ID) + return err } - *group = *resp.SecurityGroups[0] + *group = *sg return nil } diff --git a/aws/resource_aws_emr_cluster_test.go b/aws/resource_aws_emr_cluster_test.go index 9e63cc0db40..1a0ea19dc70 100644 --- a/aws/resource_aws_emr_cluster_test.go +++ b/aws/resource_aws_emr_cluster_test.go @@ -8,12 +8,12 @@ import ( "time" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/emr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/finder" ) func init() { @@ -26,7 +26,7 @@ func init() { func testSweepEmrClusters(region string) error { client, err := sharedClientForRegion(region) if err != nil { - return fmt.Errorf("error getting client: %s", err) + return fmt.Errorf("error getting client: %w", err) } conn := client.(*AWSClient).emrconn @@ -71,7 +71,7 @@ func testSweepEmrClusters(region string) error { log.Printf("[WARN] Skipping EMR Cluster sweep for %s: %s", region, err) return nil } - return fmt.Errorf("error retrieving EMR Clusters: %s", err) + return fmt.Errorf("error retrieving EMR Clusters: %w", err) } return nil @@ -1440,25 +1440,24 @@ func testAccCheckAWSEmrDestroy(s *terraform.State) error { continue } - params := &emr.DescribeClusterInput{ + input := &emr.DescribeClusterInput{ ClusterId: aws.String(rs.Primary.ID), } - describe, err := conn.DescribeCluster(params) - - if err == nil { - if describe.Cluster != nil && - *describe.Cluster.Status.State == "WAITING" { - return fmt.Errorf("EMR Cluster still exists") - } + output, err := conn.DescribeCluster(input) + if err != nil { + return err } - providerErr, ok := err.(awserr.Error) - if !ok { - return err + // if output.Cluster != nil && + // *output.Cluster.Status.State == "WAITING" { + // return fmt.Errorf("EMR Cluster still exists") + // } + if output.Cluster == nil || output.Cluster.Status == nil || aws.StringValue(output.Cluster.Status.State) == emr.ClusterStateTerminated { + continue } - log.Printf("[ERROR] %v", providerErr) + return fmt.Errorf("EMR Cluster still exists") } return nil @@ -1478,7 +1477,7 @@ func testAccCheckAWSEmrClusterExists(n string, v *emr.Cluster) resource.TestChec ClusterId: aws.String(rs.Primary.ID), }) if err != nil { - return fmt.Errorf("EMR error: %v", err) + return fmt.Errorf("EMR error: %w", err) } if describe.Cluster == nil || *describe.Cluster.Id != rs.Primary.ID { @@ -1549,7 +1548,7 @@ func testAccCheckAWSEmrClusterDisappears(cluster *emr.Cluster) resource.TestChec } if err != nil { - return fmt.Errorf("error waiting for EMR Cluster (%s) Instances to drain: %s", id, err) + return fmt.Errorf("error waiting for EMR Cluster (%s) Instances to drain: %w", id, err) } return nil @@ -1584,10 +1583,10 @@ func testAccEmrDeleteManagedSecurityGroups(conn *ec2.EC2, vpc *ec2.Vpc) error { } for groupName := range managedSecurityGroups { - securityGroup, err := testAccEmrDescribeManagedSecurityGroup(conn, vpc, groupName) + securityGroup, err := finder.SecurityGroupByNameAndVpcID(conn, groupName, aws.StringValue(vpc.VpcId)) if err != nil { - return fmt.Errorf("error describing EMR Managed Security Group (%s): %s", groupName, err) + return fmt.Errorf("error describing EMR Managed Security Group (%s): %w", groupName, err) } managedSecurityGroups[groupName] = securityGroup @@ -1603,7 +1602,7 @@ func testAccEmrDeleteManagedSecurityGroups(conn *ec2.EC2, vpc *ec2.Vpc) error { err := testAccEmrRevokeManagedSecurityGroup(conn, securityGroup) if err != nil { - return fmt.Errorf("error revoking EMR Managed Security Group (%s): %s", groupName, err) + return fmt.Errorf("error revoking EMR Managed Security Group (%s): %w", groupName, err) } } @@ -1615,40 +1614,13 @@ func testAccEmrDeleteManagedSecurityGroups(conn *ec2.EC2, vpc *ec2.Vpc) error { err := testAccEmrDeleteManagedSecurityGroup(conn, securityGroup) if err != nil { - return fmt.Errorf("error deleting EMR Managed Security Group (%s): %s", groupName, err) + return fmt.Errorf("error deleting EMR Managed Security Group (%s): %w", groupName, err) } } return nil } -func testAccEmrDescribeManagedSecurityGroup(conn *ec2.EC2, vpc *ec2.Vpc, securityGroupName string) (*ec2.SecurityGroup, error) { - input := &ec2.DescribeSecurityGroupsInput{ - Filters: []*ec2.Filter{ - { - Name: aws.String("group-name"), - Values: aws.StringSlice([]string{securityGroupName}), - }, - { - Name: aws.String("vpc-id"), - Values: []*string{vpc.VpcId}, - }, - }, - } - - output, err := conn.DescribeSecurityGroups(input) - - if err != nil { - return nil, err - } - - if output == nil || len(output.SecurityGroups) != 1 { - return nil, nil - } - - return output.SecurityGroups[0], nil -} - func testAccEmrRevokeManagedSecurityGroup(conn *ec2.EC2, securityGroup *ec2.SecurityGroup) error { input := &ec2.RevokeSecurityGroupIngressInput{ GroupId: securityGroup.GroupId, diff --git a/aws/resource_aws_globalaccelerator_endpoint_group_test.go b/aws/resource_aws_globalaccelerator_endpoint_group_test.go index 0dda503b349..aac2d49854b 100644 --- a/aws/resource_aws_globalaccelerator_endpoint_group_test.go +++ b/aws/resource_aws_globalaccelerator_endpoint_group_test.go @@ -1,6 +1,7 @@ package aws import ( + "errors" "fmt" "regexp" "testing" @@ -12,6 +13,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/terraform" + ec2finder "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/finder" "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/globalaccelerator/finder" "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) @@ -469,27 +471,18 @@ func testAccCheckGlobalAcceleratorEndpointGroupDeleteGlobalAcceleratorSecurityGr return func(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).ec2conn - input := &ec2.DescribeSecurityGroupsInput{ - Filters: buildEC2AttributeFilterList( - map[string]string{ - "group-name": "GlobalAccelerator", - "vpc-id": aws.StringValue(vpc.VpcId), - }, - ), + sg, err := ec2finder.SecurityGroupByNameAndVpcID(conn, "GlobalAccelerator", aws.StringValue(vpc.VpcId)) + var nfe *resource.NotFoundError + if errors.As(err, &nfe) { + // Already gone. + return nil } - - output, err := conn.DescribeSecurityGroups(input) if err != nil { return err } - if len(output.SecurityGroups) == 0 { - // Already gone. - return nil - } - _, err = conn.DeleteSecurityGroup(&ec2.DeleteSecurityGroupInput{ - GroupId: output.SecurityGroups[0].GroupId, + GroupId: sg.GroupId, }) if err != nil { return err diff --git a/aws/resource_aws_security_group.go b/aws/resource_aws_security_group.go index eafb6c3d7ca..76d575b981a 100644 --- a/aws/resource_aws_security_group.go +++ b/aws/resource_aws_security_group.go @@ -2,6 +2,7 @@ package aws import ( "bytes" + "errors" "fmt" "log" "sort" @@ -11,14 +12,16 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/arn" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "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/hashcode" "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" "github.com/terraform-providers/terraform-provider-aws/aws/internal/naming" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/finder" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/waiter" ) func resourceAwsSecurityGroup() *schema.Resource { @@ -256,11 +259,10 @@ func resourceAwsSecurityGroupCreate(d *schema.ResourceData, meta interface{}) er securityGroupOpts.GroupName = aws.String(groupName) var err error - log.Printf( - "[DEBUG] Security Group create configuration: %#v", securityGroupOpts) + log.Printf("[DEBUG] Security Group create configuration: %s", securityGroupOpts) createResp, err := conn.CreateSecurityGroup(securityGroupOpts) if err != nil { - return fmt.Errorf("Error creating Security Group: %s", err) + return fmt.Errorf("Error creating Security Group: %w", err) } d.SetId(aws.StringValue(createResp.GroupId)) @@ -268,16 +270,15 @@ func resourceAwsSecurityGroupCreate(d *schema.ResourceData, meta interface{}) er log.Printf("[INFO] Security Group ID: %s", d.Id()) // Wait for the security group to truly exist - resp, err := waitForSgToExist(conn, d.Id(), d.Timeout(schema.TimeoutCreate)) + group, err := waiter.SecurityGroupCreated(conn, d.Id(), d.Timeout(schema.TimeoutCreate)) if err != nil { return fmt.Errorf( - "Error waiting for Security Group (%s) to become available: %s", + "Error waiting for Security Group (%s) to become available: %w", d.Id(), err) } // AWS defaults all Security Groups to have an ALLOW ALL egress rule. Here we // revoke that rule, so users don't unknowingly have/use it. - group := resp.(*ec2.SecurityGroup) if group.VpcId != nil && *group.VpcId != "" { log.Printf("[DEBUG] Revoking default egress rule for Security Group for %s", d.Id()) @@ -298,9 +299,7 @@ func resourceAwsSecurityGroupCreate(d *schema.ResourceData, meta interface{}) er } if _, err = conn.RevokeSecurityGroupEgress(req); err != nil { - return fmt.Errorf( - "Error revoking default egress rule for Security Group (%s): %s", - d.Id(), err) + return fmt.Errorf("Error revoking default egress rule for Security Group (%s): %w", d.Id(), err) } log.Printf("[DEBUG] Revoking default IPv6 egress rule for Security Group for %s", d.Id()) @@ -324,10 +323,8 @@ func resourceAwsSecurityGroupCreate(d *schema.ResourceData, meta interface{}) er if err != nil { //If we have a NotFound or InvalidParameterValue, then we are trying to remove the default IPv6 egress of a non-IPv6 //enabled SG - if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() != "InvalidPermission.NotFound" && !isAWSErr(err, "InvalidParameterValue", "remote-ipv6-range") { - return fmt.Errorf( - "Error revoking default IPv6 egress rule for Security Group (%s): %s", - d.Id(), err) + if !tfawserr.ErrCodeEquals(err, "InvalidPermission.NotFound") && !tfawserr.ErrMessageContains(err, "InvalidParameterValue", "remote-ipv6-range") { + return fmt.Errorf("Error revoking default IPv6 egress rule for Security Group (%s): %w", d.Id(), err) } } @@ -340,25 +337,16 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro conn := meta.(*AWSClient).ec2conn ignoreTagsConfig := meta.(*AWSClient).IgnoreTagsConfig - var sgRaw interface{} - var err error - if d.IsNewResource() { - sgRaw, err = waitForSgToExist(conn, d.Id(), d.Timeout(schema.TimeoutRead)) - } else { - sgRaw, _, err = SGStateRefreshFunc(conn, d.Id())() - } - - if err != nil { - return err - } - - if sgRaw == nil { + sg, err := finder.SecurityGroupByID(conn, d.Id()) + var nfe *resource.NotFoundError + if errors.As(err, &nfe) { log.Printf("[WARN] Security group (%s) not found, removing from state", d.Id()) d.SetId("") return nil } - - sg := sgRaw.(*ec2.SecurityGroup) + if err != nil { + return fmt.Errorf("error reading Security Group (%s): %w", d.Id(), err) + } remoteIngressRules := resourceAwsSecurityGroupIPPermGather(d.Id(), sg.IpPermissions, sg.OwnerId) remoteEgressRules := resourceAwsSecurityGroupIPPermGather(d.Id(), sg.IpPermissionsEgress, sg.OwnerId) @@ -387,15 +375,15 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro d.Set("vpc_id", sg.VpcId) if err := d.Set("ingress", ingressRules); err != nil { - log.Printf("[WARN] Error setting Ingress rule set for (%s): %s", d.Id(), err) + return fmt.Errorf("error setting ingress: %w", err) } if err := d.Set("egress", egressRules); err != nil { - log.Printf("[WARN] Error setting Egress rule set for (%s): %s", d.Id(), err) + return fmt.Errorf("error setting egress: %w", err) } if err := d.Set("tags", keyvaluetags.Ec2KeyValueTags(sg.Tags).IgnoreAws().IgnoreConfig(ignoreTagsConfig).Map()); err != nil { - return fmt.Errorf("error setting tags: %s", err) + return fmt.Errorf("error setting tags: %w", err) } return nil @@ -404,25 +392,11 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro func resourceAwsSecurityGroupUpdate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).ec2conn - var sgRaw interface{} - var err error - if d.IsNewResource() { - sgRaw, err = waitForSgToExist(conn, d.Id(), d.Timeout(schema.TimeoutRead)) - } else { - sgRaw, _, err = SGStateRefreshFunc(conn, d.Id())() - } - + group, err := finder.SecurityGroupByID(conn, d.Id()) if err != nil { - return err - } - if sgRaw == nil { - log.Printf("[WARN] Security group (%s) not found, removing from state", d.Id()) - d.SetId("") - return nil + return fmt.Errorf("error updating Security Group (%s): %w", d.Id(), err) } - group := sgRaw.(*ec2.SecurityGroup) - err = resourceAwsSecurityGroupUpdateRules(d, "ingress", meta, group) if err != nil { return err @@ -439,7 +413,7 @@ func resourceAwsSecurityGroupUpdate(d *schema.ResourceData, meta interface{}) er o, n := d.GetChange("tags") if err := keyvaluetags.Ec2UpdateTags(conn, d.Id(), o, n); err != nil { - return fmt.Errorf("error updating EC2 Security Group (%s) tags: %s", d.Id(), err) + return fmt.Errorf("error updating EC2 Security Group (%s) tags: %w", d.Id(), err) } } @@ -452,7 +426,7 @@ func resourceAwsSecurityGroupDelete(d *schema.ResourceData, meta interface{}) er log.Printf("[DEBUG] Security Group destroy: %v", d.Id()) if err := deleteLingeringLambdaENIs(conn, "group-id", d.Id(), d.Timeout(schema.TimeoutDelete)); err != nil { - return fmt.Errorf("error deleting Lambda ENIs using Security Group (%s): %s", d.Id(), err) + return fmt.Errorf("error deleting Lambda ENIs using Security Group (%s): %w", d.Id(), err) } // conditionally revoke rules first before attempting to delete the group @@ -485,37 +459,31 @@ func resourceAwsSecurityGroupDelete(d *schema.ResourceData, meta interface{}) er } } if err != nil { - return fmt.Errorf("Error deleting security group: %s", err) + return fmt.Errorf("Error deleting security group: %w", err) } return nil } // Revoke all ingress/egress rules that a Security Group has func forceRevokeSecurityGroupRules(conn *ec2.EC2, d *schema.ResourceData) error { - sgRaw, _, err := SGStateRefreshFunc(conn, d.Id())() + group, err := finder.SecurityGroupByID(conn, d.Id()) if err != nil { return err } - if sgRaw == nil { - return nil - } - group := sgRaw.(*ec2.SecurityGroup) if len(group.IpPermissions) > 0 { req := &ec2.RevokeSecurityGroupIngressInput{ GroupId: group.GroupId, IpPermissions: group.IpPermissions, } - if group.VpcId == nil || *group.VpcId == "" { + if aws.StringValue(group.VpcId) == "" { req.GroupId = nil req.GroupName = group.GroupName } _, err = conn.RevokeSecurityGroupIngress(req) if err != nil { - return fmt.Errorf( - "Error revoking security group %s rules: %s", - *group.GroupId, err) + return fmt.Errorf("error revoking Security Group (%s) rules: %w", aws.StringValue(group.GroupId), err) } } @@ -527,9 +495,7 @@ func forceRevokeSecurityGroupRules(conn *ec2.EC2, d *schema.ResourceData) error _, err = conn.RevokeSecurityGroupEgress(req) if err != nil { - return fmt.Errorf( - "Error revoking security group %s rules: %s", - *group.GroupId, err) + return fmt.Errorf("error revoking Security Group (%s) rules: %w", aws.StringValue(group.GroupId), err) } } @@ -751,9 +717,7 @@ func resourceAwsSecurityGroupUpdateRules( } if err != nil { - return fmt.Errorf( - "Error revoking security group %s rules: %s", - ruleset, err) + return fmt.Errorf("error revoking Security Group (%s) rules: %w", ruleset, err) } } @@ -781,9 +745,7 @@ func resourceAwsSecurityGroupUpdateRules( } if err != nil { - return fmt.Errorf( - "Error authorizing security group %s rules: %s", - ruleset, err) + return fmt.Errorf("error authorizing Security Group (%s) rules: %w", ruleset, err) } } } @@ -791,50 +753,6 @@ func resourceAwsSecurityGroupUpdateRules( return nil } -// SGStateRefreshFunc returns a resource.StateRefreshFunc that is used to watch -// a security group. -func SGStateRefreshFunc(conn *ec2.EC2, id string) resource.StateRefreshFunc { - return func() (interface{}, string, error) { - req := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(id)}, - } - resp, err := conn.DescribeSecurityGroups(req) - if err != nil { - if ec2err, ok := err.(awserr.Error); ok { - if ec2err.Code() == "InvalidSecurityGroupID.NotFound" || - ec2err.Code() == "InvalidGroup.NotFound" { - resp = nil - err = nil - } - } - - if err != nil { - log.Printf("Error on SGStateRefresh: %s", err) - return nil, "", err - } - } - - if resp == nil { - return nil, "", nil - } - - group := resp.SecurityGroups[0] - return group, "exists", nil - } -} - -func waitForSgToExist(conn *ec2.EC2, id string, timeout time.Duration) (interface{}, error) { - log.Printf("[DEBUG] Waiting for Security Group (%s) to exist", id) - stateConf := &resource.StateChangeConf{ - Pending: []string{""}, - Target: []string{"exists"}, - Refresh: SGStateRefreshFunc(conn, id), - Timeout: timeout, - } - - return stateConf.WaitForState() -} - // matchRules receives the group id, type of rules, and the local / remote maps // of rules. We iterate through the local set of rules trying to find a matching // remote rule, which may be structured differently because of how AWS @@ -1419,7 +1337,7 @@ func deleteLingeringLambdaENIs(conn *ec2.EC2, filterName, resourceId string, tim }) if err != nil { - return fmt.Errorf("error describing ENIs: %s", err) + return fmt.Errorf("error describing ENIs: %w", err) } for _, eni := range resp.NetworkInterfaces { @@ -1451,7 +1369,7 @@ func deleteLingeringLambdaENIs(conn *ec2.EC2, filterName, resourceId string, tim } if err != nil { - return fmt.Errorf("error waiting for Lambda V2N ENI (%s) to become available for detachment: %s", eniId, err) + return fmt.Errorf("error waiting for Lambda V2N ENI (%s) to become available for detachment: %w", eniId, err) } eni = eniRaw.(*ec2.NetworkInterface) @@ -1460,13 +1378,13 @@ func deleteLingeringLambdaENIs(conn *ec2.EC2, filterName, resourceId string, tim err = detachNetworkInterface(conn, eni, timeout) if err != nil { - return fmt.Errorf("error detaching Lambda ENI (%s): %s", eniId, err) + return fmt.Errorf("error detaching Lambda ENI (%s): %w", eniId, err) } err = deleteNetworkInterface(conn, eniId) if err != nil { - return fmt.Errorf("error deleting Lambda ENI (%s): %s", eniId, err) + return fmt.Errorf("error deleting Lambda ENI (%s): %w", eniId, err) } } diff --git a/aws/resource_aws_security_group_rule.go b/aws/resource_aws_security_group_rule.go index 5a4e27a4197..0242186e0fd 100644 --- a/aws/resource_aws_security_group_rule.go +++ b/aws/resource_aws_security_group_rule.go @@ -10,12 +10,14 @@ import ( "time" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "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/hashcode" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/finder" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func resourceAwsSecurityGroupRule() *schema.Resource { @@ -152,7 +154,7 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{} awsMutexKV.Lock(sg_id) defer awsMutexKV.Unlock(sg_id) - sg, err := findResourceSecurityGroup(conn, sg_id) + sg, err := finder.SecurityGroupByID(conn, sg_id) if err != nil { return err } @@ -206,20 +208,15 @@ func resourceAwsSecurityGroupRuleCreate(d *schema.ResourceData, meta interface{} return fmt.Errorf("Security Group Rule must be type 'ingress' or type 'egress'") } - if autherr != nil { - if awsErr, ok := autherr.(awserr.Error); ok { - if awsErr.Code() == "InvalidPermission.Duplicate" { - return fmt.Errorf(`[WARN] A duplicate Security Group rule was found on (%s). This may be + if tfawserr.ErrCodeEquals(autherr, "InvalidPermission.Duplicate") { + return fmt.Errorf(`[WARN] A duplicate Security Group rule was found on (%s). This may be a side effect of a now-fixed Terraform issue causing two security groups with identical attributes but different source_security_group_ids to overwrite each other in the state. See https://github.com/hashicorp/terraform/pull/2376 for more -information and instructions for recovery. Error message: %s`, sg_id, awsErr.Message()) - } - } - - return fmt.Errorf( - "Error authorizing security group rule type %s: %s", - ruleType, autherr) +information and instructions for recovery. Error: %w`, sg_id, autherr) + } + if autherr != nil { + return fmt.Errorf("Error authorizing security group rule type %s: %w", ruleType, autherr) } var rules []*ec2.IpPermission @@ -227,7 +224,7 @@ information and instructions for recovery. Error message: %s`, sg_id, awsErr.Mes log.Printf("[DEBUG] Computed group rule ID %s", id) err = resource.Retry(5*time.Minute, func() *resource.RetryError { - sg, err := findResourceSecurityGroup(conn, sg_id) + sg, err := finder.SecurityGroupByID(conn, sg_id) if err != nil { log.Printf("[DEBUG] Error finding Security Group (%s) for Rule (%s): %s", sg_id, id, err) @@ -252,9 +249,9 @@ information and instructions for recovery. Error message: %s`, sg_id, awsErr.Mes return nil }) if isResourceTimeoutError(err) { - sg, err := findResourceSecurityGroup(conn, sg_id) + sg, err := finder.SecurityGroupByID(conn, sg_id) if err != nil { - return fmt.Errorf("Error finding security group: %s", err) + return fmt.Errorf("Error finding security group: %w", err) } switch ruleType { @@ -266,7 +263,7 @@ information and instructions for recovery. Error message: %s`, sg_id, awsErr.Mes rule := findRuleMatch(perm, rules, isVPC) if rule == nil { - return fmt.Errorf("Error finding matching security group rule: %s", err) + return fmt.Errorf("Error finding matching security group rule: %w", err) } } if err != nil { @@ -280,17 +277,17 @@ information and instructions for recovery. Error message: %s`, sg_id, awsErr.Mes func resourceAwsSecurityGroupRuleRead(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).ec2conn sg_id := d.Get("security_group_id").(string) - sg, err := findResourceSecurityGroup(conn, sg_id) - if _, notFound := err.(securityGroupNotFound); notFound { - // The security group containing this rule no longer exists. + sg, err := finder.SecurityGroupByID(conn, sg_id) + if tfresource.NotFound(err) { + log.Printf("[WARN] Security Group (%s) not found, removing Rule (%s) from state", sg_id, d.Id()) d.SetId("") return nil } if err != nil { - return fmt.Errorf("Error finding security group (%s) for rule (%s): %s", sg_id, d.Id(), err) + return fmt.Errorf("error finding Security Group (%s) for Rule (%s): %w", sg_id, d.Id(), err) } - isVPC := sg.VpcId != nil && *sg.VpcId != "" + isVPC := aws.StringValue(sg.VpcId) != "" var rule *ec2.IpPermission var rules []*ec2.IpPermission @@ -360,7 +357,7 @@ func resourceAwsSecurityGroupRuleDelete(d *schema.ResourceData, meta interface{} awsMutexKV.Lock(sg_id) defer awsMutexKV.Unlock(sg_id) - sg, err := findResourceSecurityGroup(conn, sg_id) + sg, err := finder.SecurityGroupByID(conn, sg_id) if err != nil { return err } @@ -382,14 +379,11 @@ func resourceAwsSecurityGroupRuleDelete(d *schema.ResourceData, meta interface{} _, err = conn.RevokeSecurityGroupIngress(req) if err != nil { - return fmt.Errorf( - "Error revoking security group %s rules: %s", - sg_id, err) + return fmt.Errorf("Error revoking security group %s rules: %w", sg_id, err) } case "egress": - log.Printf("[DEBUG] Revoking security group %#v %s rule: %#v", - sg_id, "egress", perm) + log.Printf("[DEBUG] Revoking security group %#v %s rule: %#v", sg_id, "egress", perm) req := &ec2.RevokeSecurityGroupEgressInput{ GroupId: sg.GroupId, IpPermissions: []*ec2.IpPermission{perm}, @@ -398,49 +392,13 @@ func resourceAwsSecurityGroupRuleDelete(d *schema.ResourceData, meta interface{} _, err = conn.RevokeSecurityGroupEgress(req) if err != nil { - return fmt.Errorf( - "Error revoking security group %s rules: %s", - sg_id, err) + return fmt.Errorf("Error revoking security group %s rules: %w", sg_id, err) } } return nil } -func findResourceSecurityGroup(conn *ec2.EC2, id string) (*ec2.SecurityGroup, error) { - req := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(id)}, - } - resp, err := conn.DescribeSecurityGroups(req) - if err, ok := err.(awserr.Error); ok && err.Code() == "InvalidGroup.NotFound" { - return nil, securityGroupNotFound{id, nil} - } - if err != nil { - return nil, err - } - if resp == nil { - return nil, securityGroupNotFound{id, nil} - } - if len(resp.SecurityGroups) != 1 || resp.SecurityGroups[0] == nil { - return nil, securityGroupNotFound{id, resp.SecurityGroups} - } - - return resp.SecurityGroups[0], nil -} - -type securityGroupNotFound struct { - id string - securityGroups []*ec2.SecurityGroup -} - -func (err securityGroupNotFound) Error() string { - if err.securityGroups == nil { - return fmt.Sprintf("No security group with ID %q", err.id) - } - return fmt.Sprintf("Expected to find one security group with ID %q, got: %#v", - err.id, err.securityGroups) -} - // ByGroupPair implements sort.Interface for []*ec2.UserIDGroupPairs based on // GroupID or GroupName field (only one should be set). type ByGroupPair []*ec2.UserIdGroupPair @@ -899,7 +857,7 @@ func resourceSecurityGroupRuleDescriptionUpdate(conn *ec2.EC2, d *schema.Resourc awsMutexKV.Lock(sg_id) defer awsMutexKV.Unlock(sg_id) - sg, err := findResourceSecurityGroup(conn, sg_id) + sg, err := finder.SecurityGroupByID(conn, sg_id) if err != nil { return err } @@ -919,9 +877,7 @@ func resourceSecurityGroupRuleDescriptionUpdate(conn *ec2.EC2, d *schema.Resourc _, err = conn.UpdateSecurityGroupRuleDescriptionsIngress(req) if err != nil { - return fmt.Errorf( - "Error updating security group %s rule description: %s", - sg_id, err) + return fmt.Errorf("Error updating security group %s rule description: %w", sg_id, err) } case "egress": req := &ec2.UpdateSecurityGroupRuleDescriptionsEgressInput{ @@ -932,9 +888,7 @@ func resourceSecurityGroupRuleDescriptionUpdate(conn *ec2.EC2, d *schema.Resourc _, err = conn.UpdateSecurityGroupRuleDescriptionsEgress(req) if err != nil { - return fmt.Errorf( - "Error updating security group %s rule description: %s", - sg_id, err) + return fmt.Errorf("Error updating security group %s rule description: %w", sg_id, err) } } diff --git a/aws/resource_aws_security_group_rule_test.go b/aws/resource_aws_security_group_rule_test.go index cecb8378ebf..92f8bcab819 100644 --- a/aws/resource_aws_security_group_rule_test.go +++ b/aws/resource_aws_security_group_rule_test.go @@ -10,11 +10,12 @@ import ( "testing" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ec2" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/finder" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func TestIpPermissionIDHash(t *testing.T) { @@ -663,14 +664,11 @@ func TestAccAWSSecurityGroupRule_PrefixListEgress(t *testing.T) { prefixListsOutput, err := conn.DescribePrefixLists(prefixListInput) if err != nil { - _, ok := err.(awserr.Error) - if !ok { - return fmt.Errorf("Error reading VPC Endpoint prefix list: %s", err.Error()) - } + return fmt.Errorf("error reading VPC Endpoint prefix list: %w", err) } if len(prefixListsOutput.PrefixLists) != 1 { - return fmt.Errorf("There are multiple prefix lists associated with the service name '%s'. Unexpected", prefixListsOutput) + return fmt.Errorf("unexpected multiple prefix lists associated with the service: %s", prefixListsOutput) } p = ec2.IpPermission{ @@ -1066,14 +1064,11 @@ func TestAccAWSSecurityGroupRule_MultiDescription(t *testing.T) { prefixListsOutput, err := conn.DescribePrefixLists(prefixListInput) if err != nil { - _, ok := err.(awserr.Error) - if !ok { - return fmt.Errorf("Error reading VPC Endpoint prefix list: %s", err.Error()) - } + return fmt.Errorf("error reading VPC Endpoint prefix list: %w", err) } if len(prefixListsOutput.PrefixLists) != 1 { - return fmt.Errorf("There are multiple prefix lists associated with the service name '%s'. Unexpected", prefixListsOutput) + return fmt.Errorf("unexpected multiple prefix lists associated with the service: %s", prefixListsOutput) } rule4 = ec2.IpPermission{ @@ -1188,27 +1183,15 @@ func testAccCheckAWSSecurityGroupRuleDestroy(s *terraform.State) error { continue } - // Retrieve our group - req := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(rs.Primary.ID)}, - } - resp, err := conn.DescribeSecurityGroups(req) - if err == nil { - if len(resp.SecurityGroups) > 0 && *resp.SecurityGroups[0].GroupId == rs.Primary.ID { - return fmt.Errorf("Security Group (%s) still exists.", rs.Primary.ID) - } - - return nil - } - - ec2err, ok := err.(awserr.Error) - if !ok { - return err + _, err := finder.SecurityGroupByID(conn, rs.Primary.ID) + if tfresource.NotFound(err) { + continue } - // Confirm error code is what we want - if ec2err.Code() != "InvalidGroup.NotFound" { + if err != nil { return err } + + return fmt.Errorf("Security Group (%s) still exists.", rs.Primary.ID) } return nil @@ -1226,20 +1209,15 @@ func testAccCheckAWSSecurityGroupRuleExists(n string, group *ec2.SecurityGroup) } conn := testAccProvider.Meta().(*AWSClient).ec2conn - req := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(rs.Primary.ID)}, - } - resp, err := conn.DescribeSecurityGroups(req) + + sg, err := finder.SecurityGroupByID(conn, rs.Primary.ID) if err != nil { return err } - if len(resp.SecurityGroups) > 0 && *resp.SecurityGroups[0].GroupId == rs.Primary.ID { - *group = *resp.SecurityGroups[0] - return nil - } + *group = *sg - return fmt.Errorf("Security Group not found") + return nil } } diff --git a/aws/resource_aws_security_group_test.go b/aws/resource_aws_security_group_test.go index 6bb0204bdba..f52ac0a91ed 100644 --- a/aws/resource_aws_security_group_test.go +++ b/aws/resource_aws_security_group_test.go @@ -12,14 +12,14 @@ import ( "time" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/ec2" - "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "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/terraform" "github.com/terraform-providers/terraform-provider-aws/aws/internal/naming" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/ec2/finder" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) // add sweeper to delete known test sgs @@ -36,7 +36,7 @@ func init() { func testSweepSecurityGroups(region string) error { client, err := sharedClientForRegion(region) if err != nil { - return fmt.Errorf("error getting client: %s", err) + return fmt.Errorf("error getting client: %w", err) } conn := client.(*AWSClient).ec2conn @@ -82,7 +82,7 @@ func testSweepSecurityGroups(region string) error { } if err != nil { - return fmt.Errorf("Error retrieving EC2 Security Groups: %s", err) + return fmt.Errorf("Error retrieving EC2 Security Groups: %w", err) } err = conn.DescribeSecurityGroupsPages(input, func(page *ec2.DescribeSecurityGroupsOutput, lastPage bool) bool { @@ -118,7 +118,7 @@ func testSweepSecurityGroups(region string) error { }) if err != nil { - return fmt.Errorf("Error retrieving EC2 Security Groups: %s", err) + return fmt.Errorf("Error retrieving EC2 Security Groups: %w", err) } return nil @@ -2108,15 +2108,11 @@ func testAddRuleCycle(primary, secondary *ec2.SecurityGroup) resource.TestCheckF var err error _, err = conn.AuthorizeSecurityGroupEgress(req1) if err != nil { - return fmt.Errorf( - "Error authorizing primary security group %s rules: %s", *primary.GroupId, - err) + return fmt.Errorf("Error authorizing primary security group %s rules: %w", aws.StringValue(primary.GroupId), err) } _, err = conn.AuthorizeSecurityGroupEgress(req2) if err != nil { - return fmt.Errorf( - "Error authorizing secondary security group %s rules: %s", *secondary.GroupId, - err) + return fmt.Errorf("Error authorizing secondary security group %s rules: %w", aws.StringValue(secondary.GroupId), err) } return nil } @@ -2143,9 +2139,7 @@ func testRemoveRuleCycle(primary, secondary *ec2.SecurityGroup) resource.TestChe } if _, err = conn.RevokeSecurityGroupIngress(req); err != nil { - return fmt.Errorf( - "Error revoking default ingress rule for Security Group in testRemoveCycle (%s): %s", - *primary.GroupId, err) + return fmt.Errorf("Error revoking default ingress rule for Security Group in testRemoveCycle (%s): %w", aws.StringValue(primary.GroupId), err) } } @@ -2156,9 +2150,7 @@ func testRemoveRuleCycle(primary, secondary *ec2.SecurityGroup) resource.TestChe } if _, err = conn.RevokeSecurityGroupEgress(req); err != nil { - return fmt.Errorf( - "Error revoking default egress rule for Security Group in testRemoveCycle (%s): %s", - *sg.GroupId, err) + return fmt.Errorf("Error revoking default egress rule for Security Group in testRemoveCycle (%s): %w", aws.StringValue(sg.GroupId), err) } } } @@ -2174,27 +2166,15 @@ func testAccCheckAWSSecurityGroupDestroy(s *terraform.State) error { continue } - // Retrieve our group - req := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(rs.Primary.ID)}, - } - resp, err := conn.DescribeSecurityGroups(req) - if err == nil { - if len(resp.SecurityGroups) > 0 && *resp.SecurityGroups[0].GroupId == rs.Primary.ID { - return fmt.Errorf("Security Group (%s) still exists.", rs.Primary.ID) - } - - return nil - } - - ec2err, ok := err.(awserr.Error) - if !ok { - return err + _, err := finder.SecurityGroupByID(conn, rs.Primary.ID) + if tfresource.NotFound(err) { + continue } - // Confirm error code is what we want - if ec2err.Code() != "InvalidGroup.NotFound" { + if err != nil { return err } + + return fmt.Errorf("Security Group (%s) still exists.", rs.Primary.ID) } return nil @@ -2208,25 +2188,15 @@ func testAccCheckAWSSecurityGroupEc2ClassicDestroy(s *terraform.State) error { continue } - input := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(rs.Primary.ID)}, - } - - output, err := conn.DescribeSecurityGroups(input) - - if tfawserr.ErrCodeEquals(err, "InvalidGroup.NotFound") { + _, err := finder.SecurityGroupByID(conn, rs.Primary.ID) + if tfresource.NotFound(err) { continue } - if err != nil { - return fmt.Errorf("error describing EC2 Security Group (%s): %w", rs.Primary.ID, err) + return err } - for _, sg := range output.SecurityGroups { - if aws.StringValue(sg.GroupId) == rs.Primary.ID { - return fmt.Errorf("EC2 Security Group (%s) still exists", rs.Primary.ID) - } - } + return fmt.Errorf("Security Group (%s) still exists.", rs.Primary.ID) } return nil @@ -2244,20 +2214,18 @@ func testAccCheckAWSSecurityGroupExists(n string, group *ec2.SecurityGroup) reso } conn := testAccProvider.Meta().(*AWSClient).ec2conn - req := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(rs.Primary.ID)}, + + sg, err := finder.SecurityGroupByID(conn, rs.Primary.ID) + if tfresource.NotFound(err) { + return fmt.Errorf("Security Group (%s) not found: %w", rs.Primary.ID, err) } - resp, err := conn.DescribeSecurityGroups(req) if err != nil { return err } - if len(resp.SecurityGroups) > 0 && *resp.SecurityGroups[0].GroupId == rs.Primary.ID { - *group = *resp.SecurityGroups[0] - return nil - } + *group = *sg - return fmt.Errorf("Security Group not found") + return nil } } @@ -2274,24 +2242,17 @@ func testAccCheckAWSSecurityGroupEc2ClassicExists(n string, group *ec2.SecurityG conn := testAccProviderEc2Classic.Meta().(*AWSClient).ec2conn - input := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(rs.Primary.ID)}, + sg, err := finder.SecurityGroupByID(conn, rs.Primary.ID) + if tfresource.NotFound(err) { + return fmt.Errorf("Security Group (%s) not found: %w", rs.Primary.ID, err) } - - output, err := conn.DescribeSecurityGroups(input) - if err != nil { - return fmt.Errorf("error describing EC2 Security Group (%s): %w", rs.Primary.ID, err) + return err } - for _, sg := range output.SecurityGroups { - if aws.StringValue(sg.GroupId) == rs.Primary.ID { - *group = *sg - return nil - } - } + *group = *sg - return fmt.Errorf("EC2 Security Group (%s) not found", rs.Primary.ID) + return nil } } @@ -2543,20 +2504,17 @@ func testAccCheckAWSSecurityGroupExistsWithoutDefault(n string) resource.TestChe } conn := testAccProvider.Meta().(*AWSClient).ec2conn - req := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(rs.Primary.ID)}, + + group, err := finder.SecurityGroupByID(conn, rs.Primary.ID) + if tfresource.NotFound(err) { + return fmt.Errorf("Security Group (%s) not found: %w", rs.Primary.ID, err) } - resp, err := conn.DescribeSecurityGroups(req) if err != nil { return err } - if len(resp.SecurityGroups) > 0 && *resp.SecurityGroups[0].GroupId == rs.Primary.ID { - group := *resp.SecurityGroups[0] - - if len(group.IpPermissionsEgress) != 1 { - return fmt.Errorf("Security Group should have only 1 egress rule, got %d", len(group.IpPermissionsEgress)) - } + if len(group.IpPermissionsEgress) != 1 { + return fmt.Errorf("Security Group should have only 1 egress rule, got %d", len(group.IpPermissionsEgress)) } return nil @@ -2667,26 +2625,18 @@ func TestAccAWSSecurityGroup_ruleLimitCidrBlockExceededAppend(t *testing.T) { t.Fatalf("PreConfig check failed: %s", err) } - id := *group.GroupId + id := aws.StringValue(group.GroupId) conn := testAccProvider.Meta().(*AWSClient).ec2conn - req := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(id)}, + + match, err := finder.SecurityGroupByID(conn, id) + if tfresource.NotFound(err) { + t.Fatalf("PreConfig check failed: Security Group (%s) not found: %s", id, err) } - resp, err := conn.DescribeSecurityGroups(req) if err != nil { t.Fatalf("PreConfig check failed: %s", err) } - var match *ec2.SecurityGroup - if len(resp.SecurityGroups) > 0 && *resp.SecurityGroups[0].GroupId == id { - match = resp.SecurityGroups[0] - } - - if match == nil { - t.Fatalf("PreConfig check failed: security group %s not found", id) - } - if cidrCount := len(match.IpPermissionsEgress[0].IpRanges); cidrCount != ruleLimit { t.Fatalf("PreConfig check failed: rule does not have previous IP ranges, has %d", cidrCount) } @@ -2833,23 +2783,15 @@ func testAccCheckAWSSecurityGroupRuleCount(group *ec2.SecurityGroup, expectedIng func testSecurityGroupRuleCount(id string, expectedIngressCount, expectedEgressCount int) error { conn := testAccProvider.Meta().(*AWSClient).ec2conn - req := &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String(id)}, + + group, err := finder.SecurityGroupByID(conn, id) + if tfresource.NotFound(err) { + return fmt.Errorf("Security Group (%s) not found: %w", id, err) } - resp, err := conn.DescribeSecurityGroups(req) if err != nil { return err } - var group *ec2.SecurityGroup - if len(resp.SecurityGroups) > 0 && *resp.SecurityGroups[0].GroupId == id { - group = resp.SecurityGroups[0] - } - - if group == nil { - return fmt.Errorf("Security group %s not found", id) - } - if actual := len(group.IpPermissions); actual != expectedIngressCount { return fmt.Errorf("Security group ingress rule count %d does not match %d", actual, expectedIngressCount) }