resource/aws_route: Support route import #5687

YakDriver · 2018-08-27T15:43:24Z

Fixes Add support for importing VPC route table routes #5689
Fixes Updating network_interface_id in aws_route fails #2270
(Originally part of PR resource/aws_route_table: Fix route table import to avoid deleting routes #5657)

Changes proposed in this pull request:

Support importing AWS_ROUTE resources

Output from acceptance testing:

$ 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.13s)
=== RUN   TestAccAWSRoute_importIPv6IGW
--- PASS: TestAccAWSRoute_importIPv6IGW (51.61s)
=== RUN   TestAccAWSRoute_importIPv6NetworkInterface
--- PASS: TestAccAWSRoute_importIPv6NetworkInterface (192.53s)
=== RUN   TestAccAWSRoute_importWithMultipleRTs
--- PASS: TestAccAWSRoute_importWithMultipleRTs (45.24s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	340.554s

bflad

Hi @YakDriver 👋 Thanks for splitting this out!

I think we should take a look at creating an Importer.State function that simplifies this process (example below) and also avoid having a second format for the resource ID. While the resource ID change is likely an inconsequential change since it would be non-trivial to use downstream in its current form with HashString, we do not really have a precedent for changing resource IDs. For now, I think we should just avoid changing it in case we ever do need some process or reason to change it in the future so we do not need to handle two possible resource ID formats in existing Terraform states (which is also not obvious without updating the resource SchemaVersion).

Please reach out with any questions or if you do not have time to implement this feedback, thanks!

bflad · 2018-08-27T16:51:23Z

aws/resource_aws_route.go

@@ -28,6 +27,9 @@ func resourceAwsRoute() *schema.Resource {
 		Update: resourceAwsRouteUpdate,
 		Delete: resourceAwsRouteDelete,
 		Exists: resourceAwsRouteExists,
+		Importer: &schema.ResourceImporter{
+			State: resourceAwsRouteImportState,


To support resource import when the Terraform resource ID is not usable for the resource Read function, it only requires setting up the necessary Terraform state attributes so the resource Read function can work. As written, a lot of the changes in this pull request are extraneous unless we are planning on using the resource ID exclusively for the read function. Given this resource code looks very outdated for current best practices and that this pull request creates a second possibility for the format of the resource ID in Terraform states out in the wild, I would recommend only doing the bare minimum here.

To that last point, we should be able support import without changing other functionality in the resource via something like:

Importer: &schema.ResourceImporter{ State: func(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { idParts := strings.Split(d.Id(), "_") if len(idParts) != 2 || idParts[0] == "" || idParts[1] == "" { return nil, fmt.Errorf("Unexpected format of ID (%q), expected ROUTETABLEID_DESTINATION", d.Id()) } routeTableID := idParts[0] destination := idParts[1] d.Set("route_table_id", routeTableID) if strings.Contains(destination, ":") { d.Set("destination_ipv6_cidr_block", destination) } else { d.Set("destination_cidr_block", destination) } d.SetId(fmt.Sprintf("r-%s%d", routeTableID, hashcode.String(destination))) return []*schema.ResourceData{d}, nil }, },

This supports resource import without the leading r- as well.

This can be acceptance tested via:

// Inside import TestStep's ImportStateIdFunc: testAccAWSRouteImportStateIdFunc(resourceName), // ... func testAccAWSRouteImportStateIdFunc(resourceName string) resource.ImportStateIdFunc { return func(s *terraform.State) (string, error) { rs, ok := s.RootModule().Resources[resourceName] if !ok { return "", fmt.Errorf("Not found: %s", resourceName) } if v, ok := rs.Primary.Attributes["destination_ipv6_cidr_block"]; ok && v != "" { return fmt.Sprintf("%s_%s", rs.Primary.Attributes["route_table_id"], v), nil } return fmt.Sprintf("%s_%s", rs.Primary.Attributes["route_table_id"], rs.Primary.Attributes["destination_cidr_block"]), nil }

See also pull requests like #5567

Cool! I was referencing the aws_route53_record so that's where some of the outdated stuff comes from. This way is cleaner.

bflad · 2018-08-27T16:54:08Z

aws/import_aws_route_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccAWSRoute_importBasic(t *testing.T) {


Since we are not creating additional configurations for testing import, the import TestStep can simply be added to existing tests. The same applies to the other new tests below. e.g.

func TestAccAWSRoute_basic(t *testing.T) { // ... existing test function ... resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, Providers: testAccProviders, CheckDestroy: testAccCheckAWSRouteDestroy, Steps: []resource.TestStep{ { Config: testAccAWSRouteBasicConfig, Check: resource.ComposeTestCheckFunc( testAccCheckAWSRouteExists("aws_route.bar", &route), testCheck, ), }, { ResourceName: "aws_route.bar", ImportState: true, ImportStateIdFunc: testAccAWSRouteImportStateIdFunc("aws_route.bar"), ImportStateVerify: true, }, }, }) }

bflad · 2018-08-27T16:56:15Z

aws/resource_aws_route.go

@@ -54,19 +57,16 @@ func resourceAwsRoute() *schema.Resource {
 			"gateway_id": {
 				Type:     schema.TypeString,
 				Optional: true,
-				Computed: true,


Was this change intentional? If so, why? Same with the other Computed: true removals. They seem unrelated to supporting resource import.

bflad · 2018-08-27T16:57:25Z

aws/resource_aws_route.go

@@ -77,13 +77,13 @@ func resourceAwsRoute() *schema.Resource {

 			"instance_owner_id": {
 				Type:     schema.TypeString,
+				Optional: true,


This attribute does not appear to be configurable in the resource Create or Update functions. Why was Optional: true added here? It also seems unrelated to supporting resource import.

bflad · 2018-08-27T16:59:17Z

aws/resource_aws_route.go

@@ -112,6 +112,12 @@ func resourceAwsRoute() *schema.Resource {

 func resourceAwsRouteCreate(d *schema.ResourceData, meta interface{}) error {
 	conn := meta.(*AWSClient).ec2conn
+
+	err := resourceAwsRouteCheckForImport(d)


During the Create function, the ResourceData will always be empty, so this is extraneous. This should be removed regardless of simplifying the import functionality.

bflad · 2018-08-27T17:00:41Z

aws/resource_aws_route.go

 	resourceAwsRouteSetResourceData(d, route)
 	return nil
 }

 func resourceAwsRouteRead(d *schema.ResourceData, meta interface{}) error {
 	conn := meta.(*AWSClient).ec2conn
+
+	err := resourceAwsRouteCheckForImport(d)


The Importer.State and Create functions should handle ID errors and setting the appropriate Terraform state required for read unless we switch to using schema.ImportStatePassthrough for Importer.State. The same applies to other usage in Update/Delete/Exists.

bflad · 2018-08-27T17:05:40Z

website/docs/r/route.html.markdown

+
+## Import
+
+Individual routes can be imported using a pseudo ID defined as r-ROUTETABLEID_DESTINATION.


If we simplify the using something like the example import function mentioned earlier, we can remove r- from import. 👍

bflad · 2018-08-27T17:07:55Z

website/docs/r/route.html.markdown

+Individual routes can be imported using a pseudo ID defined as r-ROUTETABLEID_DESTINATION.
+
+For example, to import a route in a route table with ID rtb-656C65616E6F72 and an IPv4 CIDR destination of 10.42.0.0/16,
+the import ID would be as follows:


Nitpick: We can remove the indirection of calling something the "import ID" and just show what we mean:

For example, to import a route in a route table with ID `rtb-656C65616E6F72` and an IPv4 CIDR destination of `10.42.0.0/16`: ``` $ terraform import aws_route.my_route rtb-656C65616E6F72_10.42.0.0/16 ``` To import a route from route table `rtb-656C65616E6F72` with an IPv6 destination CIDR of `2620:0:2d0:200::8/125`: ``` $ terraform import aws_route.my_route rtb-656C65616E6F72_2620:0:2d0:200::8/125 ```

YakDriver · 2018-08-28T00:05:13Z

@bflad I've implemented the changes. Let me know if you see anything else.

YakDriver · 2018-08-28T00:07:16Z

New tests...

$ make testacc TESTARGS='-run=TestAccAWSRoute_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSRoute_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSRoute_basic
--- PASS: TestAccAWSRoute_basic (50.57s)
=== RUN   TestAccAWSRoute_ipv6Support
--- PASS: TestAccAWSRoute_ipv6Support (41.06s)
=== RUN   TestAccAWSRoute_ipv6ToInternetGateway
--- PASS: TestAccAWSRoute_ipv6ToInternetGateway (45.98s)
=== RUN   TestAccAWSRoute_ipv6ToInstance
--- PASS: TestAccAWSRoute_ipv6ToInstance (139.54s)
=== RUN   TestAccAWSRoute_ipv6ToNetworkInterface
--- PASS: TestAccAWSRoute_ipv6ToNetworkInterface (179.74s)
=== RUN   TestAccAWSRoute_ipv6ToPeeringConnection
--- PASS: TestAccAWSRoute_ipv6ToPeeringConnection (41.01s)
=== RUN   TestAccAWSRoute_changeRouteTable
--- PASS: TestAccAWSRoute_changeRouteTable (75.50s)
=== RUN   TestAccAWSRoute_changeCidr
--- PASS: TestAccAWSRoute_changeCidr (80.10s)
=== RUN   TestAccAWSRoute_noopdiff
--- PASS: TestAccAWSRoute_noopdiff (120.35s)
=== RUN   TestAccAWSRoute_doesNotCrashWithVPCEndpoint
--- PASS: TestAccAWSRoute_doesNotCrashWithVPCEndpoint (55.98s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	829.873s

Enable use of standard import mechanism to import aws_route resources. The enhancement was complicated by AWS not assigning route table routes (aws_route) an ID. However, a route can be uniquely identified with a route table ID and CIDR destination. Thus, creating a pseudo ID defined by r-ROUTETABLEID_CIDRDESTINATION allows routes to be identified and imported. Related hashicorp#5631, #704, hashicorp/terraform#13779

bflad

LGTM -- thanks @YakDriver! 🚀

10 tests passed (all tests)
--- PASS: TestAccAWSRoute_ipv6ToPeeringConnection (19.75s)
--- PASS: TestAccAWSRoute_ipv6Support (21.28s)
--- PASS: TestAccAWSRoute_basic (30.94s)
--- PASS: TestAccAWSRoute_ipv6ToInternetGateway (31.97s)
--- PASS: TestAccAWSRoute_doesNotCrashWithVPCEndpoint (34.51s)
--- PASS: TestAccAWSRoute_changeRouteTable (42.86s)
--- PASS: TestAccAWSRoute_changeCidr (53.14s)
--- PASS: TestAccAWSRoute_ipv6ToInstance (100.42s)
--- PASS: TestAccAWSRoute_noopdiff (173.03s)
--- PASS: TestAccAWSRoute_ipv6ToNetworkInterface (175.56s)

bflad · 2018-08-30T23:12:40Z

This has been released in version 1.34.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

Import route tables using inline routes in the resource data. This is the same way resource data is structured when routes tables are read (and created) which enables imports to line up properly with existing resources. Previously, if you applied a state that included the import of a route table, all routes in the route table would be deleted. This bug occurred because the import function (resourceAwsRouteTableImportState()) would return a target state including both a route table resource and separate route resources for each route. The route table read function (resourceAwsRouteTableRead()) returns a state with a route table resource having inline routes. Despite being equivalent, since the states did not match, Terraform would delete all routes in the route table when applying the change plan. Fixes hashicorp#5631 Update functions names to comply with convention This commit is planned to occur after PR hashicorp#5687 which changes the names of these functions. In order to avoid merge conflicts at that time, this pre-emptively renames the functions.

ghost · 2020-04-03T17:50:46Z

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!

YakDriver mentioned this pull request Aug 27, 2018

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

Merged

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. labels Aug 27, 2018

This was referenced Aug 27, 2018

Importing routes & ACL rules #704

Closed

Add support for importing VPC route table routes #5689

Closed

bflad reviewed Aug 27, 2018

View reviewed changes

YakDriver force-pushed the support-route-import branch 3 times, most recently from 706624f to ccaf208 Compare August 28, 2018 00:03

YakDriver force-pushed the support-route-import branch from ccaf208 to 5e8f990 Compare August 28, 2018 16:01

YakDriver added 3 commits August 29, 2018 13:48

resource/aws_route: Update the import documentation

b906213

resource/aws_route: Change function names to conform to convention

3404a0b

YakDriver force-pushed the support-route-import branch from 5e8f990 to 3404a0b Compare August 29, 2018 17:48

YakDriver changed the title ~~Support route import~~ resource/aws_route: Support route import Aug 29, 2018

bflad added this to the v1.34.0 milestone Aug 30, 2018

bflad approved these changes Aug 30, 2018

View reviewed changes

bflad merged commit 4ff0de7 into hashicorp:master Aug 30, 2018

bflad added a commit that referenced this pull request Aug 30, 2018

Update CHANGELOG for #5687

b1e7c4d

YakDriver mentioned this pull request Nov 8, 2018

aws_route import reports success even on fail #6404

Closed

ghost locked and limited conversation to collaborators Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_route: Support route import #5687

resource/aws_route: Support route import #5687

YakDriver commented Aug 27, 2018 •

edited

Loading

bflad left a comment

bflad Aug 27, 2018

YakDriver Aug 27, 2018

bflad Aug 27, 2018

bflad Aug 27, 2018

bflad Aug 27, 2018

bflad Aug 27, 2018

bflad Aug 27, 2018

bflad Aug 27, 2018

bflad Aug 27, 2018

YakDriver commented Aug 28, 2018

YakDriver commented Aug 28, 2018

bflad left a comment

bflad commented Aug 30, 2018

ghost commented Apr 3, 2020


		## Import

		Individual routes can be imported using a pseudo ID defined as r-ROUTETABLEID_DESTINATION.

resource/aws_route: Support route import #5687

resource/aws_route: Support route import #5687

Conversation

YakDriver commented Aug 27, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YakDriver commented Aug 28, 2018

YakDriver commented Aug 28, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Aug 30, 2018

ghost commented Apr 3, 2020

YakDriver commented Aug 27, 2018 •

edited

Loading