From 3f472eb2b932fb81b4ad16a2d87c88560a87917a Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 22 Aug 2018 17:39:33 -0400 Subject: [PATCH 1/2] Fix route table importing to avoid route deletion 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 #5631 Update functions names to comply with convention This commit is planned to occur after PR #5687 which changes the names of these functions. In order to avoid merge conflicts at that time, this pre-emptively renames the functions. --- aws/import_aws_route_table.go | 107 -------------------------------- aws/resource_aws_route_table.go | 13 +++- 2 files changed, 12 insertions(+), 108 deletions(-) delete mode 100644 aws/import_aws_route_table.go diff --git a/aws/import_aws_route_table.go b/aws/import_aws_route_table.go deleted file mode 100644 index 6934245a299..00000000000 --- a/aws/import_aws_route_table.go +++ /dev/null @@ -1,107 +0,0 @@ -package aws - -import ( - "fmt" - "log" - - "github.com/aws/aws-sdk-go/service/ec2" - "github.com/hashicorp/terraform/helper/schema" -) - -// Route table import also imports all the rules -func resourceAwsRouteTableImportState( - d *schema.ResourceData, - meta interface{}) ([]*schema.ResourceData, error) { - conn := meta.(*AWSClient).ec2conn - - // First query the resource itself - id := d.Id() - resp, err := conn.DescribeRouteTables(&ec2.DescribeRouteTablesInput{ - RouteTableIds: []*string{&id}, - }) - if err != nil { - return nil, err - } - if len(resp.RouteTables) < 1 || resp.RouteTables[0] == nil { - return nil, fmt.Errorf("route table %s is not found", id) - } - table := resp.RouteTables[0] - - // Start building our results - results := make([]*schema.ResourceData, 1, - 2+len(table.Associations)+len(table.Routes)) - results[0] = d - log.Print("[WARN] RouteTable imports will be handled differently in a future version.") - log.Printf("[WARN] This import will create %d resources (aws_route_table, aws_route, aws_route_table_association).", len(results)) - log.Print("[WARN] In the future, only 1 aws_route_table resource will be created with inline routes.") - - { - // Construct the routes - subResource := resourceAwsRoute() - for _, route := range table.Routes { - // Ignore the local/default route - if route.GatewayId != nil && *route.GatewayId == "local" { - continue - } - - if route.Origin != nil && *route.Origin == "EnableVgwRoutePropagation" { - continue - } - - if route.DestinationPrefixListId != nil { - // Skipping because VPC endpoint routes are handled separately - // See aws_vpc_endpoint - continue - } - - // Minimal data for route - d := subResource.Data(nil) - d.SetType("aws_route") - d.Set("route_table_id", id) - d.Set("destination_cidr_block", route.DestinationCidrBlock) - d.Set("destination_ipv6_cidr_block", route.DestinationIpv6CidrBlock) - d.SetId(resourceAwsRouteID(d, route)) - results = append(results, d) - } - } - - { - // Construct the associations - subResource := resourceAwsRouteTableAssociation() - for _, assoc := range table.Associations { - if *assoc.Main { - // Ignore - continue - } - - // Minimal data for route - d := subResource.Data(nil) - d.SetType("aws_route_table_association") - d.Set("route_table_id", assoc.RouteTableId) - d.SetId(*assoc.RouteTableAssociationId) - results = append(results, d) - } - } - - { - // Construct the main associations. We could do this above but - // I keep this as a separate section since it is a separate resource. - subResource := resourceAwsMainRouteTableAssociation() - for _, assoc := range table.Associations { - if !*assoc.Main { - // Ignore - continue - } - - // Minimal data for route - d := subResource.Data(nil) - d.SetType("aws_main_route_table_association") - d.Set("route_table_id", id) - d.Set("vpc_id", table.VpcId) - d.SetId(*assoc.RouteTableAssociationId) - results = append(results, d) - } - } - - return results, nil -} diff --git a/aws/resource_aws_route_table.go b/aws/resource_aws_route_table.go index a2010ce09f9..fb4b5b7e90f 100644 --- a/aws/resource_aws_route_table.go +++ b/aws/resource_aws_route_table.go @@ -21,7 +21,18 @@ func resourceAwsRouteTable() *schema.Resource { Update: resourceAwsRouteTableUpdate, Delete: resourceAwsRouteTableDelete, Importer: &schema.ResourceImporter{ - State: resourceAwsRouteTableImportState, + State: func(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { + log.Printf("[INFO] Importing Route Table ID: %s", d.Id()) + + err := resourceAwsRouteTableRead(d, meta) + if err != nil { + return nil, err + } + + results := make([]*schema.ResourceData, 1) + results[0] = d + return results, nil + }, }, Schema: map[string]*schema.Schema{ From 68f28fae6cd6c12ba09226553caa3d4aec7df857 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 22 Aug 2018 21:21:39 -0400 Subject: [PATCH 2/2] Improve acceptance tests for route table import Use checks that align with inline routes in the state. Improve tests to avoid network values that will conflict with fewer AWS accounts. Previously, route table import acceptance tests only checked the length of the state (1 for route table, and 1 for each separate route resource). This simple test did not check much, especially when checking route tables with inline routes. Now, the state is checked to make sure it has routes and a route- count attribute that matches to expected value. --- aws/import_aws_route_table_test.go | 77 ++++++++++++++++++++++-------- 1 file changed, 57 insertions(+), 20 deletions(-) diff --git a/aws/import_aws_route_table_test.go b/aws/import_aws_route_table_test.go index 6b4ac316b4d..5a6ab852a91 100644 --- a/aws/import_aws_route_table_test.go +++ b/aws/import_aws_route_table_test.go @@ -10,9 +10,10 @@ import ( func TestAccAWSRouteTable_importBasic(t *testing.T) { checkFn := func(s []*terraform.InstanceState) error { - // Expect 2: group, 1 rules - if len(s) != 2 { - return fmt.Errorf("bad states: %#v", s) + // Expect, 1 resource in state, and route count to be 1 + v, ok := s[0].Attributes["route.#"] + if len(s) != 1 || !ok || v != "1" { + return fmt.Errorf("bad state: %s", s) } return nil @@ -36,11 +37,12 @@ func TestAccAWSRouteTable_importBasic(t *testing.T) { }) } -func TestAccAWSRouteTable_complex(t *testing.T) { +func TestAccAWSRouteTable_importWithCreate(t *testing.T) { checkFn := func(s []*terraform.InstanceState) error { - // Expect 3: group, 2 rules - if len(s) != 3 { - return fmt.Errorf("bad states: %#v", s) + // Expect, 1 resource in state, and route count to be 1 + v, ok := s[0].Attributes["route.#"] + if len(s) != 1 || !ok || v != "1" { + return fmt.Errorf("bad state: %s", s) } return nil @@ -52,7 +54,42 @@ func TestAccAWSRouteTable_complex(t *testing.T) { CheckDestroy: testAccCheckRouteTableDestroy, Steps: []resource.TestStep{ { - Config: testAccRouteTableConfig_complexImport, + Config: testAccRouteTableConfig, + ImportStateCheck: checkFn, + }, + + { + ResourceName: "aws_route_table.foo", + ImportState: true, + ImportStateVerify: true, + }, + + { + Config: testAccRouteTableConfig, + ImportStateCheck: checkFn, + }, + }, + }) +} + +func TestAccAWSRouteTable_importComplex(t *testing.T) { + checkFn := func(s []*terraform.InstanceState) error { + // Expect, 1 resource in state, and route count to be 2 + v, ok := s[0].Attributes["route.#"] + if len(s) != 1 || !ok || v != "2" { + return fmt.Errorf("bad state: %s", s) + } + + return nil + } + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckRouteTableDestroy, + Steps: []resource.TestStep{ + { + Config: testAccRouteTableConfigImportComplex, }, { @@ -64,9 +101,9 @@ func TestAccAWSRouteTable_complex(t *testing.T) { }) } -const testAccRouteTableConfig_complexImport = ` -resource "aws_vpc" "default" { - cidr_block = "10.0.0.0/16" +const testAccRouteTableConfigImportComplex = ` +resource "aws_vpc" "foo" { + cidr_block = "10.1.0.0/16" enable_dns_hostnames = true tags = { @@ -75,8 +112,8 @@ resource "aws_vpc" "default" { } resource "aws_subnet" "tf_test_subnet" { - vpc_id = "${aws_vpc.default.id}" - cidr_block = "10.0.0.0/24" + vpc_id = "${aws_vpc.foo.id}" + cidr_block = "10.1.0.0/24" map_public_ip_on_launch = true tags = { @@ -86,11 +123,11 @@ resource "aws_subnet" "tf_test_subnet" { resource "aws_eip" "nat" { vpc = true - associate_with_private_ip = "10.0.0.10" + associate_with_private_ip = "10.1.0.10" } resource "aws_internet_gateway" "gw" { - vpc_id = "${aws_vpc.default.id}" + vpc_id = "${aws_vpc.foo.id}" tags = { Name = "terraform-testacc-route-table-import-complex-default" @@ -98,7 +135,7 @@ resource "aws_internet_gateway" "gw" { } variable "private_subnet_cidrs" { - default = "10.0.0.0/24" + default = "10.1.0.0/24" } resource "aws_nat_gateway" "nat" { @@ -113,7 +150,7 @@ resource "aws_nat_gateway" "nat" { resource "aws_route_table" "mod" { count = "${length(split(",", var.private_subnet_cidrs))}" - vpc_id = "${aws_vpc.default.id}" + vpc_id = "${aws_vpc.foo.id}" tags = { Name = "tf-rt-import-test" @@ -135,7 +172,7 @@ resource "aws_route" "mod" { } resource "aws_vpc_endpoint" "s3" { - vpc_id = "${aws_vpc.default.id}" + vpc_id = "${aws_vpc.foo.id}" service_name = "com.amazonaws.us-west-2.s3" route_table_ids = ["${aws_route_table.mod.*.id}"] } @@ -143,7 +180,7 @@ resource "aws_vpc_endpoint" "s3" { ### vpc bar resource "aws_vpc" "bar" { - cidr_block = "10.1.0.0/16" + cidr_block = "10.2.0.0/16" tags = { Name = "terraform-testacc-route-table-import-complex-bar" @@ -161,7 +198,7 @@ resource "aws_internet_gateway" "ogw" { ### vpc peer connection resource "aws_vpc_peering_connection" "foo" { - vpc_id = "${aws_vpc.default.id}" + vpc_id = "${aws_vpc.foo.id}" peer_vpc_id = "${aws_vpc.bar.id}" peer_owner_id = "187416307283"