resource/aws_route_table: Fix route table import to avoid deleting routes

[YakDriver]

Fixes #5631, fixes #2270, fixes #5686

Changes proposed in this pull request:

• [BUG FIX] Make importing route tables consistent with reading route tables so that imports match existing existing resources (which avoids deletion of existing routes on apply)
• Update acceptance tests:
  • Test for deleting routes bug
  • Update existing acceptance tests for the improved consistency
  • Run in AWS accounts with fewer network conflicts

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSRouteTable_import'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSRouteTable_import -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSRouteTable_importBasic
--- PASS: TestAccAWSRouteTable_importBasic (50.20s)
=== RUN   TestAccAWSRouteTable_importWithCreate
--- PASS: TestAccAWSRouteTable_importWithCreate (62.47s)
=== RUN   TestAccAWSRouteTable_importComplex
--- PASS: TestAccAWSRouteTable_importComplex (188.63s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	301.337s

[bflad]

Marking as breaking change as this fundamentally changes the behavior of import for the resource.

I personally do not know the historical context of why this resource is setup to perform the complex import, but if I had to guess off top of my head, its because there is currently no method to import the separate aws_route resources.

[YakDriver]

Add the ability to import routes. Here are the new acceptance tests directed specifically at aws_route import:

$ make testacc TESTARGS='-run=TestAccAWSRoute_import'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSRoute_import -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSRoute_importBasic
--- PASS: TestAccAWSRoute_importBasic (51.14s)
=== RUN   TestAccAWSRoute_importIPv6IGW
--- PASS: TestAccAWSRoute_importIPv6IGW (51.20s)
=== RUN   TestAccAWSRoute_importIPv6NetworkInterface
--- PASS: TestAccAWSRoute_importIPv6NetworkInterface (177.98s)
=== RUN   TestAccAWSRoute_importWithMultipleRTs
--- PASS: TestAccAWSRoute_importWithMultipleRTs (47.10s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	327.456s

[YakDriver]

@bflad As you mentioned earlier, this change might be wise to consider at the same time as implementing importing aws_route resources. This PR now implements that enhancement.

This PR does fundamentally change the import of aws_route_table. However, I disagree that it is a breaking change. Here is why.

Currently, if you create an aws_route_table with an inline route, you'll end up with a state having an aws_route_table and an inline route. We'd agree that you can manage your aws_route_table from Terraform in that situation. It's not an anomalous state. I'd guess it's a situation a good chunk of, if not most, users find themselves in. I wouldn't say they are in a "breaking" condition.

If down the road, you import that exact same aws_route_table, you'll end up with a state having an aws_route_table and an aws_route. You might expect (as we did) to have the same state after the import as you did after create, but, alas, you do not. When Terraform compares your current state to the import state, it compares apples to oranges by comparing aws_route_table with inline routes (the read/current state) to aws_route_table + aws_route (the import state). The bug forces you into an anomalous state and, as a result, your routes get deleted.

This has production impact on us. When managing tenant infrastructures, when we import route tables as part of a larger import, make changes to unrelated resources, and then attempt to apply the changes, all the routes are deleted. That is the breaking issue. :)

[bflad]

@YakDriver I do really want to emphasize that this change does seem like a good path forward, its just a matter of keeping our compatibility promises. The aws_route_table import behavior has been like this for a very long time though and people may be depending on its current behavior. The maintainers are not likely to merge a change in this type of behavior before a major release, which is coming in the next few weeks to coincide with the Terraform 0.12 beta.

You have a few options for working with this today:

• Continue with the existing code and use the following workaround: terraform state rm the newly added aws_route resources found via terraform plan if you would like to only manage via aws_route_table
• Build and run a custom provider binary with the code changes contained in this pull request

My recommendations for this pull request in its current state to move things forward (unless we want to do all this in 2.0.0):

• Split the two code changes here:
  • resource/aws_route: Support resource import (this can be reviewed, adjusted, and merged today)
  • resource/aws_route_table: Remove aws_route resource import (this can be reviewed, adjusted and merged in 2.0.0)
• Create a separate PR to document the upcoming change in the Version 2 Upgrade Guide and provide [WARN] logs indicating the change of behavior. We may also want to consider properly documenting the existing behavior and available workaround(s) in the aws_route_table resource documentation, even if we are deprecating it.

[YakDriver]

@bflad The aws_route import PR #5687 is ready to go. I'll work on cleaning up the other two PRs and update this when they are ready to go.

• resource/aws_route: Support resource import resource/aws_route: Support route import #5687 (ready for review - branch: support-route-import)
• resource/aws_route_table: Remove aws_route import resource/aws_route_table: Fix route table import to avoid deleting routes #5657 (ready for review - branch: fix-route-table-import)
• resource/aws_route_table: Document upcoming changes (resource/aws_route_table: Add documentation/warnings for import behavior #5715) (ready for review - branch: route-table-bug-docs)
  • Document upcoming change in Version 2 Upgrade Guide
  • Provide [WARN] logs indicating the change in behavior
  • Document current behavior and workaround(s) in aws_route_table resource documentation

[YakDriver]

Test on master: If you run the new acceptance that checks to see if the route is deleted after import/apply on master, it fails.

$ make testacc TESTARGS='-run=TestAccAWSRouteTable_importWithCreate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSRouteTable_importWithCreate -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSRouteTable_importWithCreate
--- FAIL: TestAccAWSRouteTable_importWithCreate (51.20s)
	testing.go:527: Step 1 error: Failed state verification, resource with ID r-rtb-0dd113e4fe3107bdd1218385255 not found
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	51.232s
make: *** [testacc] Error 1

Test on this branch: Acceptance tests after adjusting, rebasing.

$ make testacc TESTARGS='-run=TestAccAWSRouteTable_import'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSRouteTable_import -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSRouteTable_importBasic
--- PASS: TestAccAWSRouteTable_importBasic (53.04s)
=== RUN   TestAccAWSRouteTable_importWithCreate
--- PASS: TestAccAWSRouteTable_importWithCreate (68.20s)
=== RUN   TestAccAWSRouteTable_importComplex
--- PASS: TestAccAWSRouteTable_importComplex (186.16s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	307.443s

[YakDriver]

@bflad Everything you suggested for implementation is ready for review, adjust, merge. Please see earlier comment for PR #s.

[YakDriver]

This PR needs some work. Currently, with this PR:

1. Config: route table + inline routes
   Import: route table + inline routes
   After apply: Correct (TF reports no changes needed)
2. Config: route table + separate "correctly named" aws_route resources
   Import: route table + inline routes + separate resources
   After apply: No routes deleted but 400 errors because routes already exist (because of separate resources, and none are in the state, tries to create them)
   Expected: Import results in route table + separate resources and TF reports no changes needed and no 400 errors because of trying to create existing resources
3. Config: route table + separate incorrectly named aws_route resources
   Import: route table + inline routes + separate resources
   After apply: No routes deleted but 400 errors because routes already exist (because of separate resources, and none are in the state, tries to create them)
   Expected: Import results in route table + separate resources and TF reports no changes needed and no 400 errors because of trying to create existing resources

Here are configs based on these scenarios that, in my opinion, should work:

• PR makes this work now.

resource "aws_route_table" "test" {
  vpc_id = "${aws_vpc.test.id}"

  route {
    cidr_block = "0.0.0.0/0"
    gateway_id = "${aws_internet_gateway.test.id}"
  }

  tags {
    Name = "bflad-testing"
  }
}

• No deletion but you get 400 errors (recreating existing routes)

resource "aws_route_table" "test" {
  vpc_id = "${aws_vpc.test.id}"

  tags {
    Name = "bflad-testing"
  }
}

resource "aws_route" "test" {
  route_table_id         = "${aws_route_table.test.id}"
  destination_cidr_block = "0.0.0.0/0"
  gateway_id             = "${aws_internet_gateway.test.id}"
}

• No deletion but you get 400 errors (recreating existing routes)

resource "aws_route_table" "test" {
  vpc_id = "${aws_vpc.test.id}"

  tags {
    Name = "bflad-testing"
  }
}

resource "aws_route" "randomname" {
  route_table_id         = "${aws_route_table.test.id}"
  destination_cidr_block = "0.0.0.0/0"
  gateway_id             = "${aws_internet_gateway.test.id}"
}

[bflad]

#5715 has been merged so this is ready for rebase 👍

This pull request should not worry about handling scenarios with separate aws_route resources in configurations -- its up to the operator to import the separate resource if they choose to manage their configuration in this manner. The new behavior of not including the other resources during import is more consistent with the import behavior of other resources in the provider. 😄

[bflad]

That's by design, not a problem.

The route attribute is configured as Optional and Computed:

https://github.com/terraform-providers/terraform-provider-aws/blob/ee18327b50777d6f5e3bc06277cd723c5953ef2d/aws/resource_aws_route_table.go#L44-L48

This is what allows there to be a separate aws_route resource to begin with. The lack of providing a route configuration will show no difference in the aws_route_table resource. It does not matter that the routes are saved into the Terraform state as they have no bearing unless you start configuring them in your configuration.

---

As an aside: the import process for a resource is an implicit call to the read function of a resource after the Importer.State function is called. The import process does not currently look at or care about the configuration, only getting information into the Terraform state.

In practice, this is why we generally only care about setting the ID or required attributes for reading in the Importer.State functions for resources -- we allow the read function to do all the work for the rest of the Terraform state and best practices for attributes are to always re-read them into the Terraform state during the read function for drift detection: https://www.terraform.io/docs/extend/best-practices/detecting-drift.html

While we currently support this "complex" import, where the import of one resource creates other resources in the Terraform state, its usage is very limited and often more confusing. At some point we may remove support for these "complex" imports or support better import models in coordination with upstream changes in Terraform core. There have been design sketches where Terraform automatically creates the configuration. The implementation of a feature like that may influence how import works in the future. We'll see. 😄

[YakDriver]

@bflad I guess I'll stop complaining about my PR then! Thanks

[bflad]

Thank you very much for your dedication and patience here, @YakDriver! We're currently in the process of pulling version 2.0.0 related changes and this one is ready to go. 🚀

Output from acceptance testing:

--- PASS: TestAccAWSRouteTable_panicEmptyRoute (15.79s)
--- PASS: TestAccAWSRouteTable_ipv6 (25.46s)
--- PASS: TestAccAWSRouteTable_vpcPeering (36.18s)
--- PASS: TestAccAWSRouteTable_tags (36.75s)
--- PASS: TestAccAWSRouteTable_vgwRoutePropagation (37.36s)
--- PASS: TestAccAWSRouteTable_importBasic (37.53s)
--- PASS: TestAccAWSRouteTable_importWithCreate (48.96s)
--- PASS: TestAccAWSRouteTable_basic (50.77s)
--- PASS: TestAccAWSRouteTable_instance (150.77s)
--- PASS: TestAccAWSRouteTable_Route_TransitGatewayID (290.56s)

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!