Skip to content

Commit

Permalink
Fix AWS network ACL ICMP rules changing on every plan/apply
Browse files Browse the repository at this point in the history 
When ICMP rules are present and from_port or to_port are set to non-zero
values, terraform always sees the rules as changed and recreates them
(issue: hashicorp#4423).

This change forces the affected values to zero for ICMP rules when
creating a hash of the ACL values to detect if a change is needed.

Values are hashed in the same order as before, so users should not see
any change unless they were affected by this issue already; affected
users should see only one change, after which the new hash values will
be stored in the .tfstate file.

Documentation updated to clarify that a 0 value is needed (as it
is possible to guess "-1" after looking at the protocol "all" value).
  • Loading branch information
Lars Janssen committed Mar 12, 2016
1 parent ba45f44 commit 8553b29
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 15 deletions.
20 changes: 14 additions & 6 deletions builtin/providers/aws/resource_aws_network_acl.go
View file
Expand Up @@ -464,23 +464,31 @@ func resourceAwsNetworkAclDelete(d *schema.ResourceData, meta interface{}) error

func resourceAwsNetworkAclEntryHash(v interface{}) int {
var buf bytes.Buffer
var protocolNumString string

m := v.(map[string]interface{})
buf.WriteString(fmt.Sprintf("%d-", m["from_port"].(int)))
buf.WriteString(fmt.Sprintf("%d-", m["to_port"].(int)))
buf.WriteString(fmt.Sprintf("%d-", m["rule_no"].(int)))
buf.WriteString(fmt.Sprintf("%s-", m["action"].(string)))

// The AWS network ACL API only speaks protocol numbers, and that's
// all we store. Never hash a protocol name.
protocol := m["protocol"].(string)
if _, err := strconv.Atoi(m["protocol"].(string)); err != nil {
// We're a protocol name. Look up the number.
buf.WriteString(fmt.Sprintf("%d-", protocolIntegers()[protocol]))
protocolNumString = fmt.Sprintf("%d", protocolIntegers()[protocol])
} else {
// We're a protocol number. Pass the value through.
buf.WriteString(fmt.Sprintf("%s-", protocol))
protocolNumString = fmt.Sprintf("%s", protocol)
}

if protocolNumString == "1" {
buf.WriteString("0-0-")
} else {
buf.WriteString(fmt.Sprintf("%d-", m["from_port"].(int)))
buf.WriteString(fmt.Sprintf("%d-", m["to_port"].(int)))
}

buf.WriteString(fmt.Sprintf("%d-", m["rule_no"].(int)))
buf.WriteString(fmt.Sprintf("%s-", m["action"].(string)))
buf.WriteString(fmt.Sprintf("%s-", protocolNumString))
buf.WriteString(fmt.Sprintf("%s-", m["cidr_block"].(string)))

if v, ok := m["ssl_certificate_id"]; ok {
Expand Down
112 changes: 104 additions & 8 deletions builtin/providers/aws/resource_aws_network_acl_test.go
View file
Expand Up @@ -96,7 +96,7 @@ func TestAccAWSNetworkAcl_OnlyIngressRules_update(t *testing.T) {
Config: testAccAWSNetworkAclIngressConfig,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSNetworkAclExists("aws_network_acl.foos", &networkAcl),
testIngressRuleLength(&networkAcl, 2),
testRuleLength(&networkAcl, false, 2),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.2048097841.protocol", "6"),
resource.TestCheckResourceAttr(
Expand All @@ -119,7 +119,7 @@ func TestAccAWSNetworkAcl_OnlyIngressRules_update(t *testing.T) {
Config: testAccAWSNetworkAclIngressConfigChange,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSNetworkAclExists("aws_network_acl.foos", &networkAcl),
testIngressRuleLength(&networkAcl, 1),
testRuleLength(&networkAcl, false, 1),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.2048097841.protocol", "6"),
resource.TestCheckResourceAttr(
Expand Down Expand Up @@ -157,6 +157,62 @@ func TestAccAWSNetworkAcl_OnlyEgressRules(t *testing.T) {
})
}

func TestAccAWSNetworkAcl_IcmpRules(t *testing.T) {
var networkAcl ec2.NetworkAcl

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSNetworkAclDestroy,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccAWSNetworkAclIcmpConfig,
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSNetworkAclExists("aws_network_acl.foos", &networkAcl),
testRuleLength(&networkAcl, false, 2),
testRuleLength(&networkAcl, true, 1),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.716364269.rule_no", "1"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.716364269.protocol", "1"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.2189002010.rule_no", "2"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.2189002010.protocol", "1"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.2189002010.from_port", "0"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.2189002010.to_port", "0"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.2189002010.icmp_type", "8"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.2189002010.icmp_code", "0"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.2189002010.action", "deny"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "ingress.2189002010.cidr_block", "10.4.0.0/18"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "egress.63033256.rule_no", "1"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "egress.63033256.protocol", "1"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "egress.63033256.icmp_type", "0"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "egress.63033256.icmp_code", "-1"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "egress.63033256.icmp_type", "0"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "egress.63033256.icmp_code", "-1"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "egress.63033256.action", "allow"),
resource.TestCheckResourceAttr(
"aws_network_acl.foos", "egress.63033256.cidr_block", "0.0.0.0/0"),
),
},
},
})
}

func TestAccAWSNetworkAcl_SubnetChange(t *testing.T) {

resource.Test(t, resource.TestCase{
Expand Down Expand Up @@ -286,18 +342,18 @@ func testAccCheckAWSNetworkAclExists(n string, networkAcl *ec2.NetworkAcl) resou
}
}

func testIngressRuleLength(networkAcl *ec2.NetworkAcl, length int) resource.TestCheckFunc {
func testRuleLength(networkAcl *ec2.NetworkAcl, egress bool, length int) resource.TestCheckFunc {
return func(s *terraform.State) error {
var ingressEntries []*ec2.NetworkAclEntry
var entries []*ec2.NetworkAclEntry
for _, e := range networkAcl.Entries {
if *e.Egress == false {
ingressEntries = append(ingressEntries, e)
if *e.Egress == egress {
entries = append(entries, e)
}
}
// There is always a default rule (ALL Traffic ... DENY)
// so we have to increase the length by 1
if len(ingressEntries) != length+1 {
return fmt.Errorf("Invalid number of ingress entries found; count = %d", len(ingressEntries))
if len(entries) != length+1 {
return fmt.Errorf("Invalid number of ingress entries found; count = %d", len(entries))
}
return nil
}
Expand Down Expand Up @@ -491,6 +547,46 @@ resource "aws_network_acl" "bar" {
}
}
`

const testAccAWSNetworkAclIcmpConfig = `
resource "aws_vpc" "foo" {
cidr_block = "10.1.0.0/16"
}
resource "aws_network_acl" "foos" {
vpc_id = "${aws_vpc.foo.id}"
ingress = {
protocol = "1"
rule_no = 1
action = "deny"
cidr_block = "10.2.0.0/18"
from_port = 0
to_port = 0
icmp_type = -1
icmp_code = -1
}
ingress = {
protocol = "icmp"
rule_no = 2
action = "deny"
cidr_block = "10.4.0.0/18"
from_port = 0
to_port = 0
icmp_type = 8
icmp_code = 0
}
egress = {
protocol = "icmp"
rule_no = 1
action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 0
to_port = 0
icmp_type = 0
icmp_code = -1
}
}
`

const testAccAWSNetworkAclSubnetConfig = `
resource "aws_vpc" "foo" {
cidr_block = "10.1.0.0/16"
Expand Down
5 changes: 4 additions & 1 deletion website/source/docs/providers/aws/r/network_acl.html.markdown
View file
Expand Up @@ -60,12 +60,15 @@ Both `egress` and `ingress` support the following keys:
* `action` - (Required) The action to take.
* `protocol` - (Required) The protocol to match. If using the -1 'all'
protocol, you must specify a from and to port of 0.

~> Note: You can use the protocol name or number. For more information, see here: http://www.iana.org/assignments/protocol-numbers

* `cidr_block` - (Optional) The CIDR block to match. This must be a
valid network mask.
* `icmp_type` - (Optional) The ICMP type to be used. Default 0.
* `icmp_code` - (Optional) The ICMP type code to be used. Default 0.

~> Note: For more information on ICMP types and codes, see here: http://www.nthelp.com/icmp.html
~> Note: For ICMP, `from_port` and `to_port` should always be 0. For more information on ICMP types and codes, see here: http://www.nthelp.com/icmp.html

## Attributes Reference

Expand Down

0 comments on commit 8553b29

Please sign in to comment.