Merge pull request #13766 from hashicorp/b-aws-waf-ipset-fix

provider/aws: Fix WAF IPSet descriptors removal on update

builtin/providers/aws/resource_aws_waf_ipset.go

@@ -80,17 +80,17 @@ func resourceAwsWafIPSetRead(d *schema.ResourceData, meta interface{}) error {
 		return err
 	}
 
-	var IPSetDescriptors []map[string]interface{}
+	var descriptors []map[string]interface{}
 
-	for _, IPSetDescriptor := range resp.IPSet.IPSetDescriptors {
+	for _, descriptor := range resp.IPSet.IPSetDescriptors {
-		IPSet := map[string]interface{}{
+		d := map[string]interface{}{
-			"type":  *IPSetDescriptor.Type,
+			"type":  *descriptor.Type,
-			"value": *IPSetDescriptor.Value,
+			"value": *descriptor.Value,
 		}
-		IPSetDescriptors = append(IPSetDescriptors, IPSet)
+		descriptors = append(descriptors, d)
 	}
 
-	d.Set("ip_set_descriptors", IPSetDescriptors)
+	d.Set("ip_set_descriptors", descriptors)
 
 	d.Set("name", resp.IPSet.Name)
 
@@ -98,22 +98,36 @@ func resourceAwsWafIPSetRead(d *schema.ResourceData, meta interface{}) error {
 }
 
 func resourceAwsWafIPSetUpdate(d *schema.ResourceData, meta interface{}) error {
-	err := updateIPSetResource(d, meta, waf.ChangeActionInsert)
+	conn := meta.(*AWSClient).wafconn
+
+	if d.HasChange("ip_set_descriptors") {
+		o, n := d.GetChange("ip_set_descriptors")
+		oldD, newD := o.(*schema.Set).List(), n.(*schema.Set).List()
+
+		err := updateWafIpSetDescriptors(d.Id(), oldD, newD, conn)
 		if err != nil {
 			return fmt.Errorf("Error Updating WAF IPSet: %s", err)
 		}
+	}
+
 	return resourceAwsWafIPSetRead(d, meta)
 }
 
 func resourceAwsWafIPSetDelete(d *schema.ResourceData, meta interface{}) error {
 	conn := meta.(*AWSClient).wafconn
-	err := updateIPSetResource(d, meta, waf.ChangeActionDelete)
+
+	oldDescriptors := d.Get("ip_set_descriptors").(*schema.Set).List()
+
+	if len(oldDescriptors) > 0 {
+		noDescriptors := []interface{}{}
+		err := updateWafIpSetDescriptors(d.Id(), oldDescriptors, noDescriptors, conn)
 		if err != nil {
-		return fmt.Errorf("Error Removing IPSetDescriptors: %s", err)
+			return fmt.Errorf("Error updating IPSetDescriptors: %s", err)
+		}
 	}
 
 	wr := newWafRetryer(conn, "global")
-	_, err = wr.RetryWithToken(func(token *string) (interface{}, error) {
+	_, err := wr.RetryWithToken(func(token *string) (interface{}, error) {
 		req := &waf.DeleteIPSetInput{
 			ChangeToken: token,
 			IPSetId:     aws.String(d.Id()),
@@ -128,29 +142,15 @@ func resourceAwsWafIPSetDelete(d *schema.ResourceData, meta interface{}) error {
 	return nil
 }
 
-func updateIPSetResource(d *schema.ResourceData, meta interface{}, ChangeAction string) error {
+func updateWafIpSetDescriptors(id string, oldD, newD []interface{}, conn *waf.WAF) error {
-	conn := meta.(*AWSClient).wafconn
-
 	wr := newWafRetryer(conn, "global")
 	_, err := wr.RetryWithToken(func(token *string) (interface{}, error) {
 		req := &waf.UpdateIPSetInput{
 			ChangeToken: token,
-			IPSetId:     aws.String(d.Id()),
+			IPSetId:     aws.String(id),
+			Updates:     diffWafIpSetDescriptors(oldD, newD),
 		}
-
+		log.Printf("[INFO] Updating IPSet descriptors: %s", req)
-		IPSetDescriptors := d.Get("ip_set_descriptors").(*schema.Set)
-		for _, IPSetDescriptor := range IPSetDescriptors.List() {
-			IPSet := IPSetDescriptor.(map[string]interface{})
-			IPSetUpdate := &waf.IPSetUpdate{
-				Action: aws.String(ChangeAction),
-				IPSetDescriptor: &waf.IPSetDescriptor{
-					Type:  aws.String(IPSet["type"].(string)),
-					Value: aws.String(IPSet["value"].(string)),
-				},
-			}
-			req.Updates = append(req.Updates, IPSetUpdate)
-		}
-
 		return conn.UpdateIPSet(req)
 	})
 	if err != nil {
@@ -159,3 +159,37 @@ func updateIPSetResource(d *schema.ResourceData, meta interface{}, ChangeAction
 
 	return nil
 }
+
+func diffWafIpSetDescriptors(oldD, newD []interface{}) []*waf.IPSetUpdate {
+	updates := make([]*waf.IPSetUpdate, 0)
+
+	for _, od := range oldD {
+		descriptor := od.(map[string]interface{})
+
+		if idx, contains := sliceContainsMap(newD, descriptor); contains {
+			newD = append(newD[:idx], newD[idx+1:]...)
+			continue
+		}
+
+		updates = append(updates, &waf.IPSetUpdate{
+			Action: aws.String(waf.ChangeActionDelete),
+			IPSetDescriptor: &waf.IPSetDescriptor{
+				Type:  aws.String(descriptor["type"].(string)),
+				Value: aws.String(descriptor["value"].(string)),
+			},
+		})
+	}
+
+	for _, nd := range newD {
+		descriptor := nd.(map[string]interface{})
+
+		updates = append(updates, &waf.IPSetUpdate{
+			Action: aws.String(waf.ChangeActionInsert),
+			IPSetDescriptor: &waf.IPSetDescriptor{
+				Type:  aws.String(descriptor["type"].(string)),
+				Value: aws.String(descriptor["value"].(string)),
+			},
+		})
+	}
+	return updates
+}

builtin/providers/aws/resource_aws_waf_ipset_test.go

@@ -2,6 +2,7 @@ package aws
 
 import (
 	"fmt"
+	"reflect"
 	"testing"
 
 	"github.com/hashicorp/terraform/helper/resource"
@@ -96,6 +97,169 @@ func TestAccAWSWafIPSet_changeNameForceNew(t *testing.T) {
 	})
 }
 
+func TestAccAWSWafIPSet_changeDescriptors(t *testing.T) {
+	var before, after waf.IPSet
+	ipsetName := fmt.Sprintf("ip-set-%s", acctest.RandString(5))
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSWafIPSetDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccAWSWafIPSetConfig(ipsetName),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckAWSWafIPSetExists("aws_waf_ipset.ipset", &before),
+					resource.TestCheckResourceAttr(
+						"aws_waf_ipset.ipset", "name", ipsetName),
+					resource.TestCheckResourceAttr(
+						"aws_waf_ipset.ipset", "ip_set_descriptors.#", "1"),
+					resource.TestCheckResourceAttr(
+						"aws_waf_ipset.ipset", "ip_set_descriptors.4037960608.type", "IPV4"),
+					resource.TestCheckResourceAttr(
+						"aws_waf_ipset.ipset", "ip_set_descriptors.4037960608.value", "192.0.7.0/24"),
+				),
+			},
+			{
+				Config: testAccAWSWafIPSetConfigChangeIPSetDescriptors(ipsetName),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckAWSWafIPSetExists("aws_waf_ipset.ipset", &after),
+					resource.TestCheckResourceAttr(
+						"aws_waf_ipset.ipset", "name", ipsetName),
+					resource.TestCheckResourceAttr(
+						"aws_waf_ipset.ipset", "ip_set_descriptors.#", "1"),
+					resource.TestCheckResourceAttr(
+						"aws_waf_ipset.ipset", "ip_set_descriptors.115741513.type", "IPV4"),
+					resource.TestCheckResourceAttr(
+						"aws_waf_ipset.ipset", "ip_set_descriptors.115741513.value", "192.0.8.0/24"),
+				),
+			},
+		},
+	})
+}
+
+func TestAccAWSWafIPSet_noDescriptors(t *testing.T) {
+	var ipset waf.IPSet
+	ipsetName := fmt.Sprintf("ip-set-%s", acctest.RandString(5))
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSWafIPSetDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccAWSWafIPSetConfig_noDescriptors(ipsetName),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckAWSWafIPSetExists("aws_waf_ipset.ipset", &ipset),
+					resource.TestCheckResourceAttr(
+						"aws_waf_ipset.ipset", "name", ipsetName),
+					resource.TestCheckResourceAttr(
+						"aws_waf_ipset.ipset", "ip_set_descriptors.#", "0"),
+				),
+			},
+		},
+	})
+}
+
+func TestDiffWafIpSetDescriptors(t *testing.T) {
+	testCases := []struct {
+		Old             []interface{}
+		New             []interface{}
+		ExpectedUpdates []*waf.IPSetUpdate
+	}{
+		{
+			// Change
+			Old: []interface{}{
+				map[string]interface{}{"type": "IPV4", "value": "192.0.7.0/24"},
+			},
+			New: []interface{}{
+				map[string]interface{}{"type": "IPV4", "value": "192.0.8.0/24"},
+			},
+			ExpectedUpdates: []*waf.IPSetUpdate{
+				&waf.IPSetUpdate{
+					Action: aws.String(waf.ChangeActionDelete),
+					IPSetDescriptor: &waf.IPSetDescriptor{
+						Type:  aws.String("IPV4"),
+						Value: aws.String("192.0.7.0/24"),
+					},
+				},
+				&waf.IPSetUpdate{
+					Action: aws.String(waf.ChangeActionInsert),
+					IPSetDescriptor: &waf.IPSetDescriptor{
+						Type:  aws.String("IPV4"),
+						Value: aws.String("192.0.8.0/24"),
+					},
+				},
+			},
+		},
+		{
+			// Fresh IPSet
+			Old: []interface{}{},
+			New: []interface{}{
+				map[string]interface{}{"type": "IPV4", "value": "10.0.1.0/24"},
+				map[string]interface{}{"type": "IPV4", "value": "10.0.2.0/24"},
+				map[string]interface{}{"type": "IPV4", "value": "10.0.3.0/24"},
+			},
+			ExpectedUpdates: []*waf.IPSetUpdate{
+				&waf.IPSetUpdate{
+					Action: aws.String(waf.ChangeActionInsert),
+					IPSetDescriptor: &waf.IPSetDescriptor{
+						Type:  aws.String("IPV4"),
+						Value: aws.String("10.0.1.0/24"),
+					},
+				},
+				&waf.IPSetUpdate{
+					Action: aws.String(waf.ChangeActionInsert),
+					IPSetDescriptor: &waf.IPSetDescriptor{
+						Type:  aws.String("IPV4"),
+						Value: aws.String("10.0.2.0/24"),
+					},
+				},
+				&waf.IPSetUpdate{
+					Action: aws.String(waf.ChangeActionInsert),
+					IPSetDescriptor: &waf.IPSetDescriptor{
+						Type:  aws.String("IPV4"),
+						Value: aws.String("10.0.3.0/24"),
+					},
+				},
+			},
+		},
+		{
+			// Deletion
+			Old: []interface{}{
+				map[string]interface{}{"type": "IPV4", "value": "192.0.7.0/24"},
+				map[string]interface{}{"type": "IPV4", "value": "192.0.8.0/24"},
+			},
+			New: []interface{}{},
+			ExpectedUpdates: []*waf.IPSetUpdate{
+				&waf.IPSetUpdate{
+					Action: aws.String(waf.ChangeActionDelete),
+					IPSetDescriptor: &waf.IPSetDescriptor{
+						Type:  aws.String("IPV4"),
+						Value: aws.String("192.0.7.0/24"),
+					},
+				},
+				&waf.IPSetUpdate{
+					Action: aws.String(waf.ChangeActionDelete),
+					IPSetDescriptor: &waf.IPSetDescriptor{
+						Type:  aws.String("IPV4"),
+						Value: aws.String("192.0.8.0/24"),
+					},
+				},
+			},
+		},
+	}
+	for i, tc := range testCases {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			updates := diffWafIpSetDescriptors(tc.Old, tc.New)
+			if !reflect.DeepEqual(updates, tc.ExpectedUpdates) {
+				t.Fatalf("IPSet updates don't match.\nGiven: %s\nExpected: %s",
+					updates, tc.ExpectedUpdates)
+			}
+		})
+	}
+}
+
 func testAccCheckAWSWafIPSetDisappears(v *waf.IPSet) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		conn := testAccProvider.Meta().(*AWSClient).wafconn
@@ -228,3 +392,9 @@ func testAccAWSWafIPSetConfigChangeIPSetDescriptors(name string) string {
   }
 }`, name)
 }
+
+func testAccAWSWafIPSetConfig_noDescriptors(name string) string {
+	return fmt.Sprintf(`resource "aws_waf_ipset" "ipset" {
+  name = "%s"
+}`, name)
+}

builtin/providers/aws/structure.go

@@ -2017,3 +2017,13 @@ func buildLambdaInvokeArn(lambdaArn, region string) string {
 	return fmt.Sprintf("arn:aws:apigateway:%s:lambda:path/%s/functions/%s/invocations",
 		region, apiVersion, lambdaArn)
 }
+
+func sliceContainsMap(l []interface{}, m map[string]interface{}) (int, bool) {
+	for i, t := range l {
+		if reflect.DeepEqual(m, t.(map[string]interface{})) {
+			return i, true
+		}
+	}
+
+	return -1, false
+}