r/security_group: Add option to forcefully revoke rules before deleti…

…on (#2074)

* initial work for force revoking rules, basic vpc and sg sweeper

* add docs, migration

* huh

* remove timeout for security groups

aws/import_aws_security_group_test.go

@@ -10,9 +10,9 @@ import (
 
 func TestAccAWSSecurityGroup_importBasic(t *testing.T) {
 	checkFn := func(s []*terraform.InstanceState) error {
-		// Expect 3: group, 2 rules
-		if len(s) != 3 {
-			return fmt.Errorf("expected 3 states: %#v", s)
+		// Expect 2: group, 2 rules
+		if len(s) != 2 {
+			return fmt.Errorf("expected 2 states: %#v", s)
 		}
 
 		return nil
@@ -28,9 +28,10 @@ func TestAccAWSSecurityGroup_importBasic(t *testing.T) {
 			},
 
 			{
-				ResourceName:     "aws_security_group.web",
-				ImportState:      true,
-				ImportStateCheck: checkFn,
+				ResourceName:            "aws_security_group.web",
+				ImportState:             true,
+				ImportStateCheck:        checkFn,
+				ImportStateVerifyIgnore: []string{"revoke_rules_on_delete"},
 			},
 		},
 	})
@@ -75,9 +76,10 @@ func TestAccAWSSecurityGroup_importSelf(t *testing.T) {
 			},
 
 			{
-				ResourceName:      "aws_security_group.allow_all",
-				ImportState:       true,
-				ImportStateVerify: true,
+				ResourceName:            "aws_security_group.allow_all",
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"revoke_rules_on_delete"},
 			},
 		},
 	})
@@ -94,9 +96,10 @@ func TestAccAWSSecurityGroup_importSourceSecurityGroup(t *testing.T) {
 			},
 
 			{
-				ResourceName:      "aws_security_group.test_group_1",
-				ImportState:       true,
-				ImportStateVerify: true,
+				ResourceName:            "aws_security_group.test_group_1",
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"revoke_rules_on_delete"},
 			},
 		},
 	})

aws/resource_aws_security_group.go

@@ -27,6 +27,9 @@ func resourceAwsSecurityGroup() *schema.Resource {
 			State: resourceAwsSecurityGroupImportState,
 		},
 
+		SchemaVersion: 1,
+		MigrateState:  resourceAwsSecurityGroupMigrateState,
+
 		Schema: map[string]*schema.Schema{
 			"name": {
 				Type:          schema.TypeString,
@@ -218,6 +221,12 @@ func resourceAwsSecurityGroup() *schema.Resource {
 			},
 
 			"tags": tagsSchema(),
+
+			"revoke_rules_on_delete": {
+				Type:     schema.TypeBool,
+				Default:  false,
+				Optional: true,
+			},
 		},
 	}
 }
@@ -427,6 +436,13 @@ func resourceAwsSecurityGroupDelete(d *schema.ResourceData, meta interface{}) er
 		return fmt.Errorf("Failed to delete Lambda ENIs: %s", err)
 	}
 
+	// conditionally revoke rules first before attempting to delete the group
+	if v := d.Get("revoke_rules_on_delete").(bool); v {
+		if err := forceRevokeSecurityGroupRules(conn, d); err != nil {
+			return err
+		}
+	}
+
 	return resource.Retry(5*time.Minute, func() *resource.RetryError {
 		_, err := conn.DeleteSecurityGroup(&ec2.DeleteSecurityGroupInput{
 			GroupId: aws.String(d.Id()),
@@ -453,6 +469,52 @@ func resourceAwsSecurityGroupDelete(d *schema.ResourceData, meta interface{}) er
 	})
 }
 
+// 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())()
+	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 == "" {
+			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)
+		}
+	}
+
+	if len(group.IpPermissionsEgress) > 0 {
+		req := &ec2.RevokeSecurityGroupEgressInput{
+			GroupId:       group.GroupId,
+			IpPermissions: group.IpPermissionsEgress,
+		}
+		_, err = conn.RevokeSecurityGroupEgress(req)
+
+		if err != nil {
+			return fmt.Errorf(
+				"Error revoking security group %s rules: %s",
+				*group.GroupId, err)
+		}
+	}
+
+	return nil
+}
+
 func resourceAwsSecurityGroupRuleHash(v interface{}) int {
 	var buf bytes.Buffer
 	m := v.(map[string]interface{})

aws/resource_aws_security_group_migrate.go

@@ -0,0 +1,34 @@
+package aws
+
+import (
+	"fmt"
+	"log"
+
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func resourceAwsSecurityGroupMigrateState(
+	v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) {
+	switch v {
+	case 0:
+		log.Println("[INFO] Found AWS SecurityGroup State v0; migrating to v1")
+		return migrateAwsSecurityGroupStateV0toV1(is)
+	default:
+		return is, fmt.Errorf("Unexpected schema version: %d", v)
+	}
+}
+
+func migrateAwsSecurityGroupStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) {
+	if is.Empty() || is.Attributes == nil {
+		log.Println("[DEBUG] Empty InstanceState; nothing to migrate.")
+		return is, nil
+	}
+
+	log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes)
+
+	// set default for revoke_rules_on_delete
+	is.Attributes["revoke_rules_on_delete"] = "false"
+
+	log.Printf("[DEBUG] Attributes after migration: %#v", is.Attributes)
+	return is, nil
+}

aws/resource_aws_security_group_migrate_test.go

@@ -0,0 +1,71 @@
+package aws
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform/terraform"
+)
+
+func TestAWSSecurityGroupMigrateState(t *testing.T) {
+	cases := map[string]struct {
+		StateVersion int
+		Attributes   map[string]string
+		Expected     map[string]string
+		Meta         interface{}
+	}{
+		"v0": {
+			StateVersion: 0,
+			Attributes: map[string]string{
+				"name": "test",
+			},
+			Expected: map[string]string{
+				"name": "test",
+				"revoke_rules_on_delete": "false",
+			},
+		},
+	}
+
+	for tn, tc := range cases {
+		is := &terraform.InstanceState{
+			ID:         "i-abc123",
+			Attributes: tc.Attributes,
+		}
+		is, err := resourceAwsSecurityGroupMigrateState(
+			tc.StateVersion, is, tc.Meta)
+
+		if err != nil {
+			t.Fatalf("bad: %s, err: %#v", tn, err)
+		}
+
+		for k, v := range tc.Expected {
+			if is.Attributes[k] != v {
+				t.Fatalf(
+					"bad: %s\n\n expected: %#v -> %#v\n got: %#v -> %#v\n in: %#v",
+					tn, k, v, k, is.Attributes[k], is.Attributes)
+			}
+		}
+	}
+}
+
+func TestAWSSecurityGroupMigrateState_empty(t *testing.T) {
+	var is *terraform.InstanceState
+	var meta interface{}
+
+	// should handle nil
+	is, err := resourceAwsSecurityGroupMigrateState(0, is, meta)
+
+	if err != nil {
+		t.Fatalf("err: %#v", err)
+	}
+	if is != nil {
+		t.Fatalf("expected nil instancestate, got: %#v", is)
+	}
+
+	// should handle non-nil but empty
+	is = &terraform.InstanceState{}
+	is, err = resourceAwsSecurityGroupMigrateState(0, is, meta)
+
+	if err != nil {
+		t.Fatalf("err: %#v", err)
+	}
+}