Merge pull request #26697 from hashicorp/b-sg-no-vpc

resource/aws_security_group: Use default VPC when no VPC ID passed

Author: gdavison

.changelog/26697.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_security_group: Defaults to default VPC when not supplied
+```

internal/service/ec2/vpc_security_group.go

@@ -3,7 +3,6 @@ package ec2
 import (
 	"bytes"
 	"context"
-	"errors"
 	"fmt"
 	"log"
 	"regexp"
@@ -181,10 +180,6 @@ func resourceSecurityGroupCreate(d *schema.ResourceData, meta interface{}) error
 	defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig
 	tags := defaultTagsConfig.MergeTags(tftags.New(d.Get("tags").(map[string]interface{})))
 
-	if _, ok := d.GetOk("vpc_id"); !ok {
-		return errors.New(`with the retirement of EC2-Classic no new Security Groups can be created without referencing a VPC`)
-	}
-
 	name := create.Name(d.Get("name").(string), d.Get("name_prefix").(string))
 	input := &ec2.CreateSecurityGroupInput{
 		GroupName: aws.String(name),
@@ -202,7 +197,6 @@ func resourceSecurityGroupCreate(d *schema.ResourceData, meta interface{}) error
 		input.TagSpecifications = tagSpecificationsFromKeyValueTags(tags, ec2.ResourceTypeSecurityGroup)
 	}
 
-	log.Printf("[DEBUG] Creating Security Group: %s", input)
 	output, err := conn.CreateSecurityGroup(input)
 
 	if err != nil {
@@ -220,6 +214,7 @@ func resourceSecurityGroupCreate(d *schema.ResourceData, meta interface{}) error
 
 	// AWS defaults all Security Groups to have an ALLOW ALL egress rule.
 	// Here we revoke that rule, so users don't unknowingly have/use it.
+	// This will only be false for Security Groups in EC2-Classic
 	if aws.StringValue(group.VpcId) != "" {
 		input := &ec2.RevokeSecurityGroupEgressInput{
 			GroupId: aws.String(d.Id()),
@@ -348,6 +343,7 @@ func resourceSecurityGroupUpdate(d *schema.ResourceData, meta interface{}) error
 		return fmt.Errorf("updating Security Group (%s) %s rules: %w", d.Id(), securityGroupRuleTypeIngress, err)
 	}
 
+	// This will only be false for Security Groups in EC2-Classic
 	if d.Get("vpc_id") != nil {
 		err = updateSecurityGroupRules(conn, d, securityGroupRuleTypeEgress, group)
 

internal/service/ec2/vpc_security_group_test.go

@@ -502,7 +502,7 @@ func TestAccVPCSecurityGroup_basic(t *testing.T) {
 					acctest.CheckResourceAttrAccountID(resourceName, "owner_id"),
 					resource.TestCheckResourceAttr(resourceName, "revoke_rules_on_delete", "false"),
 					resource.TestCheckResourceAttr(resourceName, "tags.%", "0"),
-					resource.TestCheckResourceAttrSet(resourceName, "vpc_id"),
+					resource.TestCheckResourceAttrPair(resourceName, "vpc_id", "aws_vpc.test", "id"),
 				),
 			},
 			{
@@ -538,6 +538,42 @@ func TestAccVPCSecurityGroup_disappears(t *testing.T) {
 	})
 }
 
+func TestAccVPCSecurityGroup_noVPC(t *testing.T) {
+	var group ec2.SecurityGroup
+	resourceName := "aws_security_group.test"
+	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.PreCheck(t) },
+		ErrorCheck:               acctest.ErrorCheck(t, ec2.EndpointsID),
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
+		CheckDestroy:             testAccCheckSecurityGroupDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccVPCSecurityGroupConfig_noVPC(rName),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckSecurityGroupExists(resourceName, &group),
+					resource.TestCheckResourceAttrPair(resourceName, "vpc_id", "data.aws_vpc.default", "id"),
+				),
+			},
+			{
+				ResourceName:            resourceName,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"revoke_rules_on_delete"},
+			},
+			{
+				Config:   testAccVPCSecurityGroupConfig_defaultVPC(rName),
+				PlanOnly: true,
+			},
+			{
+				Config:   testAccVPCSecurityGroupConfig_noVPC(rName),
+				PlanOnly: true,
+			},
+		},
+	})
+}
+
 func TestAccVPCSecurityGroup_nameGenerated(t *testing.T) {
 	var group ec2.SecurityGroup
 	resourceName := "aws_security_group.test"
@@ -1339,7 +1375,7 @@ func TestAccVPCSecurityGroup_vpc(t *testing.T) {
 						"cidr_blocks.#": "1",
 						"cidr_blocks.0": "10.0.0.0/8",
 					}),
-					resource.TestCheckResourceAttrSet(resourceName, "vpc_id"),
+					resource.TestCheckResourceAttrPair(resourceName, "vpc_id", "aws_vpc.test", "id"),
 				),
 			},
 			{
@@ -1375,7 +1411,7 @@ func TestAccVPCSecurityGroup_vpcNegOneIngress(t *testing.T) {
 						"cidr_blocks.#": "1",
 						"cidr_blocks.0": "10.0.0.0/8",
 					}),
-					resource.TestCheckResourceAttrSet(resourceName, "vpc_id"),
+					resource.TestCheckResourceAttrPair(resourceName, "vpc_id", "aws_vpc.test", "id"),
 				),
 			},
 			{
@@ -1411,7 +1447,7 @@ func TestAccVPCSecurityGroup_vpcProtoNumIngress(t *testing.T) {
 						"cidr_blocks.#": "1",
 						"cidr_blocks.0": "10.0.0.0/8",
 					}),
-					resource.TestCheckResourceAttrSet(resourceName, "vpc_id"),
+					resource.TestCheckResourceAttrPair(resourceName, "vpc_id", "aws_vpc.test", "id"),
 				),
 			},
 			{
@@ -2198,7 +2234,7 @@ func TestAccVPCSecurityGroup_emrDependencyViolation(t *testing.T) {
 					acctest.CheckResourceAttrAccountID(resourceName, "owner_id"),
 					resource.TestCheckResourceAttr(resourceName, "revoke_rules_on_delete", "true"),
 					resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
-					resource.TestCheckResourceAttrSet(resourceName, "vpc_id"),
+					resource.TestCheckResourceAttrPair(resourceName, "vpc_id", "aws_vpc.test", "id"),
 				),
 				ExpectError: regexp.MustCompile("unexpected state"),
 			},
@@ -2453,6 +2489,33 @@ resource "aws_security_group" "test" {
 `, rName, namePrefix)
 }
 
+func testAccVPCSecurityGroupConfig_noVPC(rName string) string {
+	return fmt.Sprintf(`
+resource "aws_security_group" "test" {
+  name = %[1]q
+}
+
+data "aws_vpc" "default" {
+  default = true
+}
+
+`, rName)
+}
+
+func testAccVPCSecurityGroupConfig_defaultVPC(rName string) string {
+	return fmt.Sprintf(`
+resource "aws_security_group" "test" {
+  name   = %[1]q
+  vpc_id = data.aws_vpc.default.id
+}
+
+data "aws_vpc" "default" {
+  default = true
+}
+
+`, rName)
+}
+
 func testAccVPCSecurityGroupConfig_tags1(rName, tagKey1, tagValue1 string) string {
 	return fmt.Sprintf(`
 resource "aws_vpc" "test" {

website/docs/r/security_group.html.markdown

@@ -118,14 +118,15 @@ resource "aws_security_group" "sg_with_changeable_name" {
 
 The following arguments are supported:
 
-* `description` - (Optional, Forces new resource) Security group description. Defaults to `Managed by Terraform`. Cannot be `""`. __NOTE__: This field maps to the AWS `GroupDescription` attribute, for which there is no Update API. If you'd like to classify your security groups in a way that can be updated, use `tags`.
+* `description` - (Optional, Forces new resource) Security group description. Defaults to `Managed by Terraform`. Cannot be `""`. **NOTE**: This field maps to the AWS `GroupDescription` attribute, for which there is no Update API. If you'd like to classify your security groups in a way that can be updated, use `tags`.
 * `egress` - (Optional, VPC only) Configuration block for egress rules. Can be specified multiple times for each egress rule. Each egress block supports fields documented below. This argument is processed in [attribute-as-blocks mode](https://www.terraform.io/docs/configuration/attr-as-blocks.html).
 * `ingress` - (Optional) Configuration block for ingress rules. Can be specified multiple times for each ingress rule. Each ingress block supports fields documented below. This argument is processed in [attribute-as-blocks mode](https://www.terraform.io/docs/configuration/attr-as-blocks.html).
 * `name_prefix` - (Optional, Forces new resource) Creates a unique name beginning with the specified prefix. Conflicts with `name`.
 * `name` - (Optional, Forces new resource) Name of the security group. If omitted, Terraform will assign a random, unique name.
 * `revoke_rules_on_delete` - (Optional) Instruct Terraform to revoke all of the Security Groups attached ingress and egress rules before deleting the rule itself. This is normally not needed, however certain AWS services such as Elastic Map Reduce may automatically add required rules to security groups used with the service, and those rules may contain a cyclic dependency that prevent the security groups from being destroyed without removing the dependency first. Default `false`.
 * `tags` - (Optional) Map of tags to assign to the resource. If configured with a provider [`default_tags` configuration block](https://registry.terraform.io/providers/hashicorp/aws/latest/docs#default_tags-configuration-block) present, tags with matching keys will overwrite those defined at the provider-level.
 * `vpc_id` - (Optional, Forces new resource) VPC ID.
+  Defaults to the region's default VPC.
 
 ### ingress