Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent creation of duplicate aws_network_acl_rule resources #36326

Merged
merged 16 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .changelog/36326.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
```release-note:bug
resource/aws_network_acl_rule: Prevent creation of duplicate Terraform resources
```

```release-note:bug
resource/aws_network_acl_rule: Fix `InvalidNetworkAclID.NotFound` errors on resource Delete
```

```release-note:bug
resource/aws_route: Prevent creation of duplicate Terraform resources
```

```release-note:bug
resource/aws_route_table: Fix `couldn't find resource` errors on resource Delete
```
10 changes: 10 additions & 0 deletions internal/service/ec2/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,11 @@ const (
errCodeInvalidVPNGatewayAttachmentNotFound = "InvalidVpnGatewayAttachment.NotFound"
errCodeInvalidVPNGatewayIDNotFound = "InvalidVpnGatewayID.NotFound"
errCodeNatGatewayNotFound = "NatGatewayNotFound"
errCodeNetworkACLEntryAlreadyExists = "NetworkAclEntryAlreadyExists"
errCodeOperationNotPermitted = "OperationNotPermitted"
errCodePrefixListVersionMismatch = "PrefixListVersionMismatch"
errCodeResourceNotReady = "ResourceNotReady"
errCodeRouteAlreadyExists = "RouteAlreadyExists"
errCodeSnapshotCreationPerVolumeRateExceeded = "SnapshotCreationPerVolumeRateExceeded"
errCodeUnsupportedOperation = "UnsupportedOperation"
errCodeVolumeInUse = "VolumeInUse"
Expand Down Expand Up @@ -188,3 +190,11 @@ func UnsuccessfulItemsError(apiObjects []*ec2.UnsuccessfulItem) error {

return errors.Join(errs...)
}

func networkACLEntryAlreadyExistsError(naclID string, egress bool, ruleNumber int) error {
return awserr.New(errCodeNetworkACLEntryAlreadyExists, fmt.Sprintf("EC2 Network ACL (%s) Rule (egress: %t)(%d) already exists", naclID, egress, ruleNumber), nil)
}

func routeAlreadyExistsError(routeTableID, destination string) error {
return awserr.New(errCodeRouteAlreadyExists, fmt.Sprintf("Route in Route Table (%s) with destination (%s) already exists", routeTableID, destination), nil)
}
4 changes: 4 additions & 0 deletions internal/service/ec2/exports_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@ package ec2
// Exports for use in tests only.
var (
ResourceDefaultNetworkACL = resourceDefaultNetworkACL
ResourceDefaultRouteTable = resourceDefaultRouteTable
ResourceEBSFastSnapshotRestore = newResourceEBSFastSnapshotRestore
ResourceInstanceConnectEndpoint = newResourceInstanceConnectEndpoint
ResourceNetworkACL = resourceNetworkACL
ResourceNetworkACLRule = resourceNetworkACLRule
ResourceRoute = resourceRoute
ResourceRouteTable = resourceRouteTable
ResourceSecurityGroupEgressRule = newResourceSecurityGroupEgressRule
ResourceSecurityGroupIngressRule = newResourceSecurityGroupIngressRule
ResourceTag = resourceTag
Expand Down
10 changes: 6 additions & 4 deletions internal/service/ec2/service_package_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion internal/service/ec2/vpc_default_route_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (

// @SDKResource("aws_default_route_table", name="Route Table")
// @Tags(identifierAttribute="id")
func ResourceDefaultRouteTable() *schema.Resource {
func resourceDefaultRouteTable() *schema.Resource {
return &schema.Resource{
CreateWithoutTimeout: resourceDefaultRouteTableCreate,
ReadWithoutTimeout: resourceDefaultRouteTableRead,
Expand Down
2 changes: 1 addition & 1 deletion internal/service/ec2/vpc_default_route_table_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ func TestAccVPCDefaultRouteTable_conditionalCIDRBlock(t *testing.T) {
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckRouteDestroy(ctx),
CheckDestroy: testAccCheckRouteTableDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCDefaultRouteTableConfig_conditionalIPv4v6(rName, destinationCidr, destinationIpv6Cidr, false),
Expand Down
23 changes: 16 additions & 7 deletions internal/service/ec2/vpc_network_acl_rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import (
"github.com/hashicorp/terraform-provider-aws/internal/tfresource"
)

// @SDKResource("aws_network_acl_rule")
func ResourceNetworkACLRule() *schema.Resource {
// @SDKResource("aws_network_acl_rule", name="Network ACL Rule")
func resourceNetworkACLRule() *schema.Resource {
return &schema.Resource{
CreateWithoutTimeout: resourceNetworkACLRuleCreate,
ReadWithoutTimeout: resourceNetworkACLRuleRead,
Expand Down Expand Up @@ -126,14 +126,23 @@ func resourceNetworkACLRuleCreate(ctx context.Context, d *schema.ResourceData, m

protocol := d.Get("protocol").(string)
protocolNumber, err := networkACLProtocolNumber(protocol)

if err != nil {
return sdkdiag.AppendErrorf(diags, "creating EC2 Network ACL Rule: %s", err)
}

egress := d.Get("egress").(bool)
naclID := d.Get("network_acl_id").(string)
ruleNumber := d.Get("rule_number").(int)
naclID, egress, ruleNumber := d.Get("network_acl_id").(string), d.Get("egress").(bool), d.Get("rule_number").(int)

// CreateNetworkAclEntry succeeds if there is an existing rule with identical attributes.
_, err = FindNetworkACLEntryByThreePartKey(ctx, conn, naclID, egress, ruleNumber)

switch {
case err == nil:
return sdkdiag.AppendFromErr(diags, networkACLEntryAlreadyExistsError(naclID, egress, ruleNumber))
case tfresource.NotFound(err):
break
default:
return sdkdiag.AppendErrorf(diags, "reading EC2 Network ACL Rule: %s", err)
}

input := &ec2.CreateNetworkAclEntryInput{
Egress: aws.Bool(egress),
Expand Down Expand Up @@ -242,7 +251,7 @@ func resourceNetworkACLRuleDelete(ctx context.Context, d *schema.ResourceData, m
RuleNumber: aws.Int64(int64(d.Get("rule_number").(int))),
})

if tfawserr.ErrCodeEquals(err, errCodeInvalidNetworkACLEntryNotFound) {
if tfawserr.ErrCodeEquals(err, errCodeInvalidNetworkACLIDNotFound, errCodeInvalidNetworkACLEntryNotFound) {
return diags
}

Expand Down
60 changes: 60 additions & 0 deletions internal/service/ec2/vpc_network_acl_rule_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,24 @@ func TestAccVPCNetworkACLRule_tcpProtocol(t *testing.T) {
})
}

func TestAccVPCNetworkACLRule_duplicate(t *testing.T) {
ctx := acctest.Context(t)
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckNetworkACLRuleDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCNetworkACLRuleConfig_duplicate(rName),
ExpectError: regexache.MustCompile(`NetworkAclEntryAlreadyExists: EC2 Network ACL .* already exists`),
},
},
})
}

func testAccCheckNetworkACLRuleDestroy(ctx context.Context) resource.TestCheckFunc {
return func(s *terraform.State) error {
conn := acctest.Provider.Meta().(*conns.AWSClient).EC2Conn(ctx)
Expand Down Expand Up @@ -739,6 +757,48 @@ resource "aws_network_acl" "test" {
}`, rName)
}

func testAccVPCNetworkACLRuleConfig_duplicate(rName string) string {
return fmt.Sprintf(`
resource "aws_vpc" "test" {
cidr_block = "10.3.0.0/16"

tags = {
Name = %[1]q
}
}

resource "aws_network_acl" "test" {
vpc_id = aws_vpc.test.id

tags = {
Name = %[1]q
}
}

resource "aws_network_acl_rule" "test1" {
network_acl_id = aws_network_acl.test.id
rule_number = 200
egress = false
protocol = "tcp"
rule_action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 22
to_port = 22
}

resource "aws_network_acl_rule" "test2" {
network_acl_id = aws_network_acl_rule.test1.network_acl_id
rule_number = 200
egress = false
protocol = "tcp"
rule_action = "allow"
cidr_block = "0.0.0.0/0"
from_port = 22
to_port = 22
}
`, rName)
}

func testAccNetworkACLRuleImportStateIdFunc(resourceName, resourceProtocol string) resource.ImportStateIdFunc {
return func(s *terraform.State) (string, error) {
rs, ok := s.RootModule().Resources[resourceName]
Expand Down
17 changes: 13 additions & 4 deletions internal/service/ec2/vpc_route.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ var routeValidTargets = []string{
"vpc_peering_connection_id",
}

// @SDKResource("aws_route")
func ResourceRoute() *schema.Resource {
// @SDKResource("aws_route", name="Route")
func resourceRoute() *schema.Resource {
return &schema.Resource{
CreateWithoutTimeout: resourceRouteCreate,
ReadWithoutTimeout: resourceRouteRead,
Expand Down Expand Up @@ -184,13 +184,11 @@ func resourceRouteCreate(ctx context.Context, d *schema.ResourceData, meta inter
conn := meta.(*conns.AWSClient).EC2Conn(ctx)

destinationAttributeKey, destination, err := routeDestinationAttribute(d)

if err != nil {
return sdkdiag.AppendFromErr(diags, err)
}

targetAttributeKey, target, err := routeTargetAttribute(d)

if err != nil {
return sdkdiag.AppendFromErr(diags, err)
}
Expand Down Expand Up @@ -241,6 +239,17 @@ func resourceRouteCreate(ctx context.Context, d *schema.ResourceData, meta inter
return sdkdiag.AppendErrorf(diags, "creating Route: unexpected route target attribute: %q", targetAttributeKey)
}

_, err = routeFinder(ctx, conn, routeTableID, destination)

switch {
case err == nil:
return sdkdiag.AppendFromErr(diags, routeAlreadyExistsError(routeTableID, destination))
case tfresource.NotFound(err):
break
default:
return sdkdiag.AppendErrorf(diags, "reading Route: %s", err)
}

_, err = tfresource.RetryWhenAWSErrCodeEquals(ctx, d.Timeout(schema.TimeoutCreate),
func() (interface{}, error) {
return conn.CreateRouteWithContext(ctx, input)
Expand Down
6 changes: 0 additions & 6 deletions internal/service/ec2/vpc_route_data_source_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ func TestAccVPCRouteDataSource_transitGatewayID(t *testing.T) {
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckRouteDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCRouteDataSourceConfig_ipv4TransitGateway(rName),
Expand All @@ -92,7 +91,6 @@ func TestAccVPCRouteDataSource_ipv6DestinationCIDR(t *testing.T) {
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckRouteDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCRouteDataSourceConfig_ipv6EgressOnlyInternetGateway(rName),
Expand All @@ -115,7 +113,6 @@ func TestAccVPCRouteDataSource_localGatewayID(t *testing.T) {
PreCheck: func() { acctest.PreCheck(ctx, t); acctest.PreCheckOutpostsOutposts(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckRouteDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCRouteDataSourceConfig_ipv4LocalGateway(rName),
Expand All @@ -139,7 +136,6 @@ func TestAccVPCRouteDataSource_carrierGatewayID(t *testing.T) {
PreCheck: func() { acctest.PreCheck(ctx, t); testAccPreCheckWavelengthZoneAvailable(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckRouteDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCRouteDataSourceConfig_ipv4CarrierGateway(rName),
Expand All @@ -163,7 +159,6 @@ func TestAccVPCRouteDataSource_destinationPrefixListID(t *testing.T) {
PreCheck: func() { acctest.PreCheck(ctx, t); testAccPreCheckManagedPrefixList(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckRouteDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCRouteDataSourceConfig_prefixListNATGateway(rName),
Expand All @@ -189,7 +184,6 @@ func TestAccVPCRouteDataSource_gatewayVPCEndpoint(t *testing.T) {
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckRouteDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCRouteDataSourceConfig_gatewayEndpointNo(rName),
Expand Down
7 changes: 6 additions & 1 deletion internal/service/ec2/vpc_route_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,13 @@ var routeTableValidTargets = []string{

// @SDKResource("aws_route_table", name="Route Table")
// @Tags(identifierAttribute="id")
func ResourceRouteTable() *schema.Resource {
func resourceRouteTable() *schema.Resource {
return &schema.Resource{
CreateWithoutTimeout: resourceRouteTableCreate,
ReadWithoutTimeout: resourceRouteTableRead,
UpdateWithoutTimeout: resourceRouteTableUpdate,
DeleteWithoutTimeout: resourceRouteTableDelete,

Importer: &schema.ResourceImporter{
StateContext: schema.ImportStatePassthroughContext,
},
Expand Down Expand Up @@ -353,6 +354,10 @@ func resourceRouteTableDelete(ctx context.Context, d *schema.ResourceData, meta

routeTable, err := FindRouteTableByID(ctx, conn, d.Id())

if tfresource.NotFound(err) {
return diags
}

if err != nil {
return sdkdiag.AppendErrorf(diags, "reading Route Table (%s): %s", d.Id(), err)
}
Expand Down
Loading
Loading