From 524d6e8268c2e20da72b625d516ca81cf6c5d961 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 8 Nov 2018 19:25:24 -0500 Subject: [PATCH 1/2] resource/aws_security_group_rule: Properly handle updating description when protocol is -1/ALL Previously: ``` --- FAIL: TestAccAWSSecurityGroupRule_Description_AllPorts (21.74s) testing.go:538: Step 2 error: Error applying: 1 error occurred: * aws_security_group_rule.test: 1 error occurred: * aws_security_group_rule.test: Error updating security group sg-0b6f5a54297754be3 rule description: InvalidParameterValue: When protocol is ALL, you cannot specify from-port. ``` Output from acceptance testing: ``` --- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (36.84s) --- PASS: TestAccAWSSecurityGroupRule_Egress (21.08s) --- PASS: TestAccAWSSecurityGroupRule_EgressDescription (21.13s) --- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (33.23s) --- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (1.77s) --- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (1.72s) --- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (20.29s) --- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (42.93s) --- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (41.24s) --- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (20.04s) --- PASS: TestAccAWSSecurityGroupRule_IngressDescription (20.98s) --- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (35.99s) --- PASS: TestAccAWSSecurityGroupRule_Issue5310 (20.12s) --- PASS: TestAccAWSSecurityGroupRule_MultiDescription (82.07s) --- PASS: TestAccAWSSecurityGroupRule_MultiIngress (22.57s) --- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (47.79s) --- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (44.23s) --- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (51.97s) --- PASS: TestAccAWSSecurityGroupRule_Race (274.72s) --- PASS: TestAccAWSSecurityGroupRule_SelfReference (40.94s) --- PASS: TestAccAWSSecurityGroupRule_SelfSource (41.11s) ``` --- aws/resource_aws_security_group_rule.go | 8 ++- aws/resource_aws_security_group_rule_test.go | 73 ++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/aws/resource_aws_security_group_rule.go b/aws/resource_aws_security_group_rule.go index 2a285bd289b..6784ff9940a 100644 --- a/aws/resource_aws_security_group_rule.go +++ b/aws/resource_aws_security_group_rule.go @@ -577,11 +577,15 @@ func ipPermissionIDHash(sg_id, ruleType string, ip *ec2.IpPermission) string { func expandIPPerm(d *schema.ResourceData, sg *ec2.SecurityGroup) (*ec2.IpPermission, error) { var perm ec2.IpPermission - perm.FromPort = aws.Int64(int64(d.Get("from_port").(int))) - perm.ToPort = aws.Int64(int64(d.Get("to_port").(int))) protocol := protocolForValue(d.Get("protocol").(string)) perm.IpProtocol = aws.String(protocol) + // InvalidParameterValue: When protocol is ALL, you cannot specify from-port. + if protocol != "-1" { + perm.FromPort = aws.Int64(int64(d.Get("from_port").(int))) + perm.ToPort = aws.Int64(int64(d.Get("to_port").(int))) + } + // build a group map that behaves like a set groups := make(map[string]bool) if raw, ok := d.GetOk("source_security_group_id"); ok { diff --git a/aws/resource_aws_security_group_rule_test.go b/aws/resource_aws_security_group_rule_test.go index 4c5421f11a9..68287ae84c0 100644 --- a/aws/resource_aws_security_group_rule_test.go +++ b/aws/resource_aws_security_group_rule_test.go @@ -786,6 +786,57 @@ func TestAccAWSSecurityGroupRule_EgressDescription_updates(t *testing.T) { }) } +func TestAccAWSSecurityGroupRule_Description_AllPorts(t *testing.T) { + var group ec2.SecurityGroup + rName := acctest.RandomWithPrefix("tf-acc-test") + securityGroupResourceName := "aws_security_group.test" + resourceName := "aws_security_group_rule.test" + + rule1 := ec2.IpPermission{ + IpProtocol: aws.String("-1"), + IpRanges: []*ec2.IpRange{ + {CidrIp: aws.String("0.0.0.0/0"), Description: aws.String("description1")}, + }, + } + + rule2 := ec2.IpPermission{ + IpProtocol: aws.String("-1"), + IpRanges: []*ec2.IpRange{ + {CidrIp: aws.String("0.0.0.0/0"), Description: aws.String("description2")}, + }, + } + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSSecurityGroupRuleDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSSecurityGroupRuleConfigDescriptionAllPorts(rName, "description1"), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSSecurityGroupRuleExists(securityGroupResourceName, &group), + testAccCheckAWSSecurityGroupRuleAttributes(resourceName, &group, &rule1, "ingress"), + resource.TestCheckResourceAttr(resourceName, "description", "description1"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateIdFunc: testAccAWSSecurityGroupRuleImportStateIdFunc(resourceName), + ImportStateVerify: true, + }, + { + Config: testAccAWSSecurityGroupRuleConfigDescriptionAllPorts(rName, "description2"), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSSecurityGroupRuleExists(securityGroupResourceName, &group), + testAccCheckAWSSecurityGroupRuleAttributes(resourceName, &group, &rule2, "ingress"), + resource.TestCheckResourceAttr(resourceName, "description", "description2"), + ), + }, + }, + }) +} + func TestAccAWSSecurityGroupRule_MultiDescription(t *testing.T) { var group ec2.SecurityGroup var nat ec2.SecurityGroup @@ -1726,6 +1777,28 @@ resource "aws_security_group_rule" "egress_1" { `, rInt) } +func testAccAWSSecurityGroupRuleConfigDescriptionAllPorts(rName, description string) string { + return fmt.Sprintf(` +resource "aws_security_group" "test" { + name = %q + + tags { + Name = "tf-acc-test-ec2-security-group-rule" + } +} + +resource "aws_security_group_rule" "test" { + cidr_blocks = ["0.0.0.0/0"] + description = %q + from_port = 0 + protocol = -1 + security_group_id = "${aws_security_group.test.id}" + to_port = 0 + type = "ingress" +} +`, rName, description) +} + var testAccAWSSecurityGroupRuleRace = func() string { var b bytes.Buffer iterations := 50 From 6ffadc97efe796d9ee81e016610fa6aeea7a6197 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 9 Nov 2018 13:45:59 -0500 Subject: [PATCH 2/2] tests/resource/aws_security_group_rule: Add from_port/protocol/to_port attribute checks in TestAccAWSSecurityGroupRule_Description_AllPorts ``` --- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (25.78s) ``` --- aws/resource_aws_security_group_rule_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/aws/resource_aws_security_group_rule_test.go b/aws/resource_aws_security_group_rule_test.go index 68287ae84c0..6accf4d3968 100644 --- a/aws/resource_aws_security_group_rule_test.go +++ b/aws/resource_aws_security_group_rule_test.go @@ -817,6 +817,9 @@ func TestAccAWSSecurityGroupRule_Description_AllPorts(t *testing.T) { testAccCheckAWSSecurityGroupRuleExists(securityGroupResourceName, &group), testAccCheckAWSSecurityGroupRuleAttributes(resourceName, &group, &rule1, "ingress"), resource.TestCheckResourceAttr(resourceName, "description", "description1"), + resource.TestCheckResourceAttr(resourceName, "from_port", "0"), + resource.TestCheckResourceAttr(resourceName, "protocol", "-1"), + resource.TestCheckResourceAttr(resourceName, "to_port", "0"), ), }, { @@ -831,6 +834,9 @@ func TestAccAWSSecurityGroupRule_Description_AllPorts(t *testing.T) { testAccCheckAWSSecurityGroupRuleExists(securityGroupResourceName, &group), testAccCheckAWSSecurityGroupRuleAttributes(resourceName, &group, &rule2, "ingress"), resource.TestCheckResourceAttr(resourceName, "description", "description2"), + resource.TestCheckResourceAttr(resourceName, "from_port", "0"), + resource.TestCheckResourceAttr(resourceName, "protocol", "-1"), + resource.TestCheckResourceAttr(resourceName, "to_port", "0"), ), }, },