From 81003fa6b10913dd02dbcb16a539c95e5bb3a182 Mon Sep 17 00:00:00 2001 From: Krzysztof Wilczynski Date: Sat, 16 Jul 2016 03:28:09 +0900 Subject: [PATCH 1/3] Fix icmp_type and icmp_code in aws_network_acl_rule. The ICMP type 0 (Echo Reply) was not handled correctly. This commit changes the type of attributes "icmp_type" and "icmp_code" from TypeInt to TypeString, allowing for the string value to be manually converted into an integer. This enables an integer values such as -1, 0, 8, etc., coming from the resource definition in the template to be handled correctly. Signed-off-by: Krzysztof Wilczynski --- .../aws/resource_aws_network_acl_rule.go | 21 ++++++++++++++----- .../aws/resource_aws_network_acl_rule_test.go | 14 +++++++++++-- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/builtin/providers/aws/resource_aws_network_acl_rule.go b/builtin/providers/aws/resource_aws_network_acl_rule.go index b27f908d2..be347daf3 100644 --- a/builtin/providers/aws/resource_aws_network_acl_rule.go +++ b/builtin/providers/aws/resource_aws_network_acl_rule.go @@ -63,12 +63,12 @@ func resourceAwsNetworkAclRule() *schema.Resource { ForceNew: true, }, "icmp_type": &schema.Schema{ - Type: schema.TypeInt, + Type: schema.TypeString, Optional: true, ForceNew: true, }, "icmp_code": &schema.Schema{ - Type: schema.TypeInt, + Type: schema.TypeString, Optional: true, ForceNew: true, }, @@ -103,14 +103,25 @@ func resourceAwsNetworkAclRuleCreate(d *schema.ResourceData, meta interface{}) e }, } - // Specify additional required fields for ICMP + // Specify additional required fields for ICMP. For the list + // of ICMP codes and types, see: http://www.nthelp.com/icmp.html if p == 1 { params.IcmpTypeCode = &ec2.IcmpTypeCode{} if v, ok := d.GetOk("icmp_code"); ok { - params.IcmpTypeCode.Code = aws.Int64(int64(v.(int))) + icmpCode, err := strconv.Atoi(v.(string)) + if err != nil { + return fmt.Errorf("Unable to parse ICMP code %s for rule %#v", v, d.Get("rule_number").(int)) + } + params.IcmpTypeCode.Code = aws.Int64(int64(icmpCode)) + log.Printf("[DEBUG] Transformed ICMP code %s into %d", v, icmpCode) } if v, ok := d.GetOk("icmp_type"); ok { - params.IcmpTypeCode.Type = aws.Int64(int64(v.(int))) + icmpType, err := strconv.Atoi(v.(string)) + if err != nil { + return fmt.Errorf("Unable to parse ICMP type %s for rule %#v", v, d.Get("rule_number").(int)) + } + params.IcmpTypeCode.Type = aws.Int64(int64(icmpType)) + log.Printf("[DEBUG] Transformed ICMP type %s into %d", v, icmpType) } } diff --git a/builtin/providers/aws/resource_aws_network_acl_rule_test.go b/builtin/providers/aws/resource_aws_network_acl_rule_test.go index 56973b1d4..95682bf41 100644 --- a/builtin/providers/aws/resource_aws_network_acl_rule_test.go +++ b/builtin/providers/aws/resource_aws_network_acl_rule_test.go @@ -23,7 +23,8 @@ func TestAccAWSNetworkAclRule_basic(t *testing.T) { resource.TestStep{ Config: testAccAWSNetworkAclRuleBasicConfig, Check: resource.ComposeTestCheckFunc( - testAccCheckAWSNetworkAclRuleExists("aws_network_acl_rule.bar", &networkAcl), + testAccCheckAWSNetworkAclRuleExists("aws_network_acl_rule.baz", &networkAcl), + testAccCheckAWSNetworkAclRuleExists("aws_network_acl_rule.quux", &networkAcl), ), }, }, @@ -112,7 +113,7 @@ resource "aws_vpc" "foo" { resource "aws_network_acl" "bar" { vpc_id = "${aws_vpc.foo.id}" } -resource "aws_network_acl_rule" "bar" { +resource "aws_network_acl_rule" "baz" { network_acl_id = "${aws_network_acl.bar.id}" rule_number = 200 egress = false @@ -122,4 +123,13 @@ resource "aws_network_acl_rule" "bar" { from_port = 22 to_port = 22 } +resource "aws_network_acl_rule" "quux" { + network_acl_id = "${aws_network_acl.bar.id}" + rule_number = 300 + protocol = "icmp" + rule_action = "allow" + cidr_block = "0.0.0.0/0" + icmp_type = 0 + icmp_code = -1 +} ` From 6dd0e3f6dba21148abfc0d51928f5e7897aee829 Mon Sep 17 00:00:00 2001 From: Krzysztof Wilczynski Date: Sun, 17 Jul 2016 03:24:48 +0900 Subject: [PATCH 2/3] Add validation for icmp_type and icmp_code. Also, change order of processing to parse icmp_type first. Change wording of the debug messages, and change format string type for rule_number where appropriate. Signed-off-by: Krzysztof Wilczynski --- .../aws/resource_aws_network_acl_rule.go | 49 ++++++++++++------- .../aws/resource_aws_network_acl_rule_test.go | 14 +++++- 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/builtin/providers/aws/resource_aws_network_acl_rule.go b/builtin/providers/aws/resource_aws_network_acl_rule.go index be347daf3..ad3f302a3 100644 --- a/builtin/providers/aws/resource_aws_network_acl_rule.go +++ b/builtin/providers/aws/resource_aws_network_acl_rule.go @@ -63,14 +63,16 @@ func resourceAwsNetworkAclRule() *schema.Resource { ForceNew: true, }, "icmp_type": &schema.Schema{ - Type: schema.TypeString, - Optional: true, - ForceNew: true, + Type: schema.TypeString, + Optional: true, + ForceNew: true, + ValidateFunc: validateICMPArgumentValue, }, "icmp_code": &schema.Schema{ - Type: schema.TypeString, - Optional: true, - ForceNew: true, + Type: schema.TypeString, + Optional: true, + ForceNew: true, + ValidateFunc: validateICMPArgumentValue, }, }, } @@ -85,7 +87,7 @@ func resourceAwsNetworkAclRuleCreate(d *schema.ResourceData, meta interface{}) e var ok bool p, ok = protocolIntegers()[protocol] if !ok { - return fmt.Errorf("Invalid Protocol %s for rule %#v", protocol, d.Get("rule_number").(int)) + return fmt.Errorf("Invalid Protocol %s for rule %d", protocol, d.Get("rule_number").(int)) } } log.Printf("[INFO] Transformed Protocol %s into %d", protocol, p) @@ -107,21 +109,21 @@ func resourceAwsNetworkAclRuleCreate(d *schema.ResourceData, meta interface{}) e // of ICMP codes and types, see: http://www.nthelp.com/icmp.html if p == 1 { params.IcmpTypeCode = &ec2.IcmpTypeCode{} - if v, ok := d.GetOk("icmp_code"); ok { - icmpCode, err := strconv.Atoi(v.(string)) - if err != nil { - return fmt.Errorf("Unable to parse ICMP code %s for rule %#v", v, d.Get("rule_number").(int)) - } - params.IcmpTypeCode.Code = aws.Int64(int64(icmpCode)) - log.Printf("[DEBUG] Transformed ICMP code %s into %d", v, icmpCode) - } if v, ok := d.GetOk("icmp_type"); ok { icmpType, err := strconv.Atoi(v.(string)) if err != nil { - return fmt.Errorf("Unable to parse ICMP type %s for rule %#v", v, d.Get("rule_number").(int)) + return fmt.Errorf("Unable to parse ICMP type %s for rule %d", v, d.Get("rule_number").(int)) } params.IcmpTypeCode.Type = aws.Int64(int64(icmpType)) - log.Printf("[DEBUG] Transformed ICMP type %s into %d", v, icmpType) + log.Printf("[DEBUG] Got ICMP type %d for rule %d", icmpType, d.Get("rule_number").(int)) + } + if v, ok := d.GetOk("icmp_code"); ok { + icmpCode, err := strconv.Atoi(v.(string)) + if err != nil { + return fmt.Errorf("Unable to parse ICMP code %s for rule %d", v, d.Get("rule_number").(int)) + } + params.IcmpTypeCode.Code = aws.Int64(int64(icmpCode)) + log.Printf("[DEBUG] Got ICMP code %d for rule %d", icmpCode, d.Get("rule_number").(int)) } } @@ -176,7 +178,7 @@ func resourceAwsNetworkAclRuleRead(d *schema.ResourceData, meta interface{}) err var ok bool protocol, ok := protocolStrings(protocolIntegers())[p] if !ok { - return fmt.Errorf("Invalid Protocol %s for rule %#v", *resp.Protocol, d.Get("rule_number").(int)) + return fmt.Errorf("Invalid Protocol %s for rule %d", *resp.Protocol, d.Get("rule_number").(int)) } log.Printf("[INFO] Transformed Protocol %s back into %s", *resp.Protocol, protocol) d.Set("protocol", protocol) @@ -209,7 +211,7 @@ func findNetworkAclRule(d *schema.ResourceData, meta interface{}) (*ec2.NetworkA filters := make([]*ec2.Filter, 0, 2) ruleNumberFilter := &ec2.Filter{ Name: aws.String("entry.rule-number"), - Values: []*string{aws.String(fmt.Sprintf("%v", d.Get("rule_number").(int)))}, + Values: []*string{aws.String(fmt.Sprintf("%d", d.Get("rule_number").(int)))}, } filters = append(filters, ruleNumberFilter) egressFilter := &ec2.Filter{ @@ -256,3 +258,12 @@ func networkAclIdRuleNumberEgressHash(networkAclId string, ruleNumber int, egres buf.WriteString(fmt.Sprintf("%s-", protocol)) return fmt.Sprintf("nacl-%d", hashcode.String(buf.String())) } + +func validateICMPArgumentValue(v interface{}, k string) (ws []string, errors []error) { + value := v.(string) + _, err := strconv.Atoi(value) + if len(value) == 0 || err != nil { + errors = append(errors, fmt.Errorf("%q must be an integer value: %q", k, value)) + } + return +} diff --git a/builtin/providers/aws/resource_aws_network_acl_rule_test.go b/builtin/providers/aws/resource_aws_network_acl_rule_test.go index 95682bf41..b7559d7c9 100644 --- a/builtin/providers/aws/resource_aws_network_acl_rule_test.go +++ b/builtin/providers/aws/resource_aws_network_acl_rule_test.go @@ -24,7 +24,8 @@ func TestAccAWSNetworkAclRule_basic(t *testing.T) { Config: testAccAWSNetworkAclRuleBasicConfig, Check: resource.ComposeTestCheckFunc( testAccCheckAWSNetworkAclRuleExists("aws_network_acl_rule.baz", &networkAcl), - testAccCheckAWSNetworkAclRuleExists("aws_network_acl_rule.quux", &networkAcl), + testAccCheckAWSNetworkAclRuleExists("aws_network_acl_rule.qux", &networkAcl), + testAccCheckAWSNetworkAclRuleExists("aws_network_acl_rule.wibble", &networkAcl), ), }, }, @@ -123,7 +124,7 @@ resource "aws_network_acl_rule" "baz" { from_port = 22 to_port = 22 } -resource "aws_network_acl_rule" "quux" { +resource "aws_network_acl_rule" "qux" { network_acl_id = "${aws_network_acl.bar.id}" rule_number = 300 protocol = "icmp" @@ -132,4 +133,13 @@ resource "aws_network_acl_rule" "quux" { icmp_type = 0 icmp_code = -1 } +resource "aws_network_acl_rule" "wibble" { + network_acl_id = "${aws_network_acl.bar.id}" + rule_number = 400 + protocol = "icmp" + rule_action = "allow" + cidr_block = "0.0.0.0/0" + icmp_type = -1 + icmp_code = -1 +} ` From 96b6a3dcb88aa6537376645f46f7b001338a2b3e Mon Sep 17 00:00:00 2001 From: Krzysztof Wilczynski Date: Sun, 17 Jul 2016 03:34:32 +0900 Subject: [PATCH 3/3] Add note about setting wildcard icmp_type. Signed-off-by: Krzysztof Wilczynski --- .../source/docs/providers/aws/r/network_acl_rule.html.markdown | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/website/source/docs/providers/aws/r/network_acl_rule.html.markdown b/website/source/docs/providers/aws/r/network_acl_rule.html.markdown index c5e7a327b..2df0198a6 100644 --- a/website/source/docs/providers/aws/r/network_acl_rule.html.markdown +++ b/website/source/docs/providers/aws/r/network_acl_rule.html.markdown @@ -45,6 +45,8 @@ The following arguments are supported: ~> **NOTE:** If the value of `protocol` is `-1` or `all`, the `from_port` and `to_port` values will be ignored and the rule will apply to all ports. +~> **NOTE:** If the value of `icmp_type` is `-1` (which results in a wildcard ICMP type), the `icmp_code` must also be set to `-1` (wildcard ICMP code). + ~> Note: For more information on ICMP types and codes, see here: http://www.nthelp.com/icmp.html ## Attributes Reference @@ -52,4 +54,3 @@ The following arguments are supported: The following attributes are exported: * `id` - The ID of the network ACL Rule -