From 7cc88726ab88b711baec4412c580b4508f49874f Mon Sep 17 00:00:00 2001 From: Chris Stephens Date: Wed, 10 Jul 2019 15:16:48 -0700 Subject: [PATCH 1/4] Change network connections to native go files --- ...nnection.go.erb => resource_service_networking_connection.go} | 1 - ...est.go.erb => resource_service_networking_connection_test.go} | 1 - ...tworking_operation.go.erb => service_networking_operation.go} | 1 - 3 files changed, 3 deletions(-) rename third_party/terraform/resources/{resource_service_networking_connection.go.erb => resource_service_networking_connection.go} (99%) rename third_party/terraform/tests/{resource_service_networking_connection_test.go.erb => resource_service_networking_connection_test.go} (98%) rename third_party/terraform/utils/{service_networking_operation.go.erb => service_networking_operation.go} (97%) diff --git a/third_party/terraform/resources/resource_service_networking_connection.go.erb b/third_party/terraform/resources/resource_service_networking_connection.go similarity index 99% rename from third_party/terraform/resources/resource_service_networking_connection.go.erb rename to third_party/terraform/resources/resource_service_networking_connection.go index 3535a5ba7c98..4c192d0d878c 100644 --- a/third_party/terraform/resources/resource_service_networking_connection.go.erb +++ b/third_party/terraform/resources/resource_service_networking_connection.go @@ -1,4 +1,3 @@ -<% autogen_exception -%> package google import ( diff --git a/third_party/terraform/tests/resource_service_networking_connection_test.go.erb b/third_party/terraform/tests/resource_service_networking_connection_test.go similarity index 98% rename from third_party/terraform/tests/resource_service_networking_connection_test.go.erb rename to third_party/terraform/tests/resource_service_networking_connection_test.go index 6777e0efae14..8049dc180acb 100644 --- a/third_party/terraform/tests/resource_service_networking_connection_test.go.erb +++ b/third_party/terraform/tests/resource_service_networking_connection_test.go @@ -1,4 +1,3 @@ -<% autogen_exception -%> package google import ( diff --git a/third_party/terraform/utils/service_networking_operation.go.erb b/third_party/terraform/utils/service_networking_operation.go similarity index 97% rename from third_party/terraform/utils/service_networking_operation.go.erb rename to third_party/terraform/utils/service_networking_operation.go index 8a592ac923b3..59a8c22f9f51 100644 --- a/third_party/terraform/utils/service_networking_operation.go.erb +++ b/third_party/terraform/utils/service_networking_operation.go @@ -1,4 +1,3 @@ -<% autogen_exception -%> package google import ( From bd983cd09b8d287e82a37d92e617918ff314cab3 Mon Sep 17 00:00:00 2001 From: Chris Stephens Date: Wed, 10 Jul 2019 17:10:16 -0700 Subject: [PATCH 2/4] Refactor service networking connection Change Read to remove from state instead of erroring if not found. Added the 'peering' computed filed to use as a reference key for calls to the networking resources. Support Delete by calling 'removePeering' on the associated Network --- .../resource_service_networking_connection.go | 68 +++++++++++++----- ...urce_service_networking_connection_test.go | 70 +++++++++++++++++++ 2 files changed, 121 insertions(+), 17 deletions(-) diff --git a/third_party/terraform/resources/resource_service_networking_connection.go b/third_party/terraform/resources/resource_service_networking_connection.go index 4c192d0d878c..b634774532e2 100644 --- a/third_party/terraform/resources/resource_service_networking_connection.go +++ b/third_party/terraform/resources/resource_service_networking_connection.go @@ -7,7 +7,9 @@ import ( "regexp" "strings" + "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/helper/schema" + "google.golang.org/api/compute/v1" "google.golang.org/api/servicenetworking/v1" ) @@ -44,6 +46,10 @@ func resourceServiceNetworkingConnection() *schema.Resource { Required: true, Elem: &schema.Schema{Type: schema.TypeString}, }, + "peering": &schema.Schema{ + Type: schema.TypeString, + Computed: true, + }, }, } } @@ -54,7 +60,7 @@ func resourceServiceNetworkingConnectionCreate(d *schema.ResourceData, meta inte network := d.Get("network").(string) serviceNetworkingNetworkName, err := retrieveServiceNetworkingNetworkName(d, config, network) if err != nil { - return fmt.Errorf("Failed to find Service Networking Connection, err: %s", err) + return errwrap.Wrapf("Failed to find Service Networking Connection, err: {{err}}", err) } connection := &servicenetworking.Connection{ @@ -86,18 +92,17 @@ func resourceServiceNetworkingConnectionRead(d *schema.ResourceData, meta interf connectionId, err := parseConnectionId(d.Id()) if err != nil { - return fmt.Errorf("Failed to find Service Networking Connection, err: %s", err) + return errwrap.Wrapf("Unable to parse Service Networking Connection id, err: {{err}}", err) } serviceNetworkingNetworkName, err := retrieveServiceNetworkingNetworkName(d, config, connectionId.Network) if err != nil { - return fmt.Errorf("Failed to find Service Networking Connection, err: %s", err) + return errwrap.Wrapf("Failed to find Service Networking Connection, err: {{err}}", err) } parentService := formatParentService(connectionId.Service) - listCall := config.clientServiceNetworking.Services.Connections.List(parentService) - listCall.Network(serviceNetworkingNetworkName) - response, err := listCall.Do() + response, err := config.clientServiceNetworking.Services.Connections.List(parentService). + Network(serviceNetworkingNetworkName).Do() if err != nil { return err } @@ -111,11 +116,14 @@ func resourceServiceNetworkingConnectionRead(d *schema.ResourceData, meta interf } if connection == nil { - return fmt.Errorf("Failed to find Service Networking Connection, network: %s service: %s", connectionId.Network, connectionId.Service) + d.SetId("") + log.Printf("[WARNING] Failed to find Service Networking Connection, network: %s service: %s", connectionId.Network, connectionId.Service) + return nil } d.Set("network", connectionId.Network) d.Set("service", connectionId.Service) + d.Set("peering", connection.Peering) d.Set("reserved_peering_ranges", connection.ReservedPeeringRanges) return nil } @@ -125,7 +133,7 @@ func resourceServiceNetworkingConnectionUpdate(d *schema.ResourceData, meta inte connectionId, err := parseConnectionId(d.Id()) if err != nil { - return fmt.Errorf("Failed to find Service Networking Connection, err: %s", err) + return errwrap.Wrapf("Unable to parse Service Networking Connection id, err: {{err}}", err) } parentService := formatParentService(connectionId.Service) @@ -134,7 +142,7 @@ func resourceServiceNetworkingConnectionUpdate(d *schema.ResourceData, meta inte network := d.Get("network").(string) serviceNetworkingNetworkName, err := retrieveServiceNetworkingNetworkName(d, config, network) if err != nil { - return fmt.Errorf("Failed to find Service Networking Connection, err: %s", err) + return errwrap.Wrapf("Failed to find Service Networking Connection, err: {{err}}", err) } connection := &servicenetworking.Connection{ @@ -155,18 +163,44 @@ func resourceServiceNetworkingConnectionUpdate(d *schema.ResourceData, meta inte return resourceServiceNetworkingConnectionRead(d, meta) } -// NOTE(craigatgoogle): This resource doesn't have a defined Delete method, however an un-documented -// behavior is for the Connection to be deleted when its associated network is deleted. This is -// helpeful for acctest cleanup. func resourceServiceNetworkingConnectionDelete(d *schema.ResourceData, meta interface{}) error { - connectionId, err := parseConnectionId(d.Id()) + config := meta.(*Config) + + network := d.Get("network").(string) + serviceNetworkingNetworkName, err := retrieveServiceNetworkingNetworkName(d, config, network) if err != nil { return err } - log.Printf("[WARNING] Service Networking Connection resources cannot be deleted from GCP. This Connection (network: %s, service: %s) will be removed from Terraform state, but will still be present on the server.", connectionId.Network, connectionId.Service) + obj := make(map[string]interface{}) + peering := d.Get("peering").(string) + obj["name"] = peering + url := fmt.Sprintf("%s%s/removePeering", config.ComputeBasePath, serviceNetworkingNetworkName) + + res, err := sendRequestWithTimeout(config, "POST", url, obj, d.Timeout(schema.TimeoutUpdate)) + if err != nil { + return fmt.Errorf("Error removing connection from network %s: %s", d.Id(), err) + } + + project, err := getProject(d, config) + if err != nil { + return err + } + op := &compute.Operation{} + err = Convert(res, op) + if err != nil { + return err + } + + err = computeOperationWaitTime( + config.clientCompute, op, project, "Updating Network", + int(d.Timeout(schema.TimeoutUpdate).Minutes())) + if err != nil { + return err + } d.SetId("") + log.Printf("[INFO] Service network connection removed.") return nil } @@ -202,14 +236,14 @@ func parseConnectionId(id string) (*connectionId, error) { network, err := url.QueryUnescape(res[0]) if err != nil { - return nil, fmt.Errorf("Failed to parse service networking connection id, invalid network, err: %s", err) + return nil, errwrap.Wrapf("Failed to parse service networking connection id, invalid network, err: {{err}}", err) } else if len(network) == 0 { return nil, fmt.Errorf("Failed to parse service networking connection id, empty network") } service, err := url.QueryUnescape(res[1]) if err != nil { - return nil, fmt.Errorf("Failed to parse service networking connection id, invalid service, err: %s", err) + return nil, errwrap.Wrapf("Failed to parse service networking connection id, invalid service, err: {{err}}", err) } else if len(service) == 0 { return nil, fmt.Errorf("Failed to parse service networking connection id, empty service") } @@ -226,7 +260,7 @@ func parseConnectionId(id string) (*connectionId, error) { func retrieveServiceNetworkingNetworkName(d *schema.ResourceData, config *Config, network string) (string, error) { networkFieldValue, err := ParseNetworkFieldValue(network, d, config) if err != nil { - return "", fmt.Errorf("Failed to retrieve network field value, err: %s", err) + return "", errwrap.Wrapf("Failed to retrieve network field value, err: {{err}}", err) } pid := networkFieldValue.Project diff --git a/third_party/terraform/tests/resource_service_networking_connection_test.go b/third_party/terraform/tests/resource_service_networking_connection_test.go index 8049dc180acb..774c89d11b45 100644 --- a/third_party/terraform/tests/resource_service_networking_connection_test.go +++ b/third_party/terraform/tests/resource_service_networking_connection_test.go @@ -6,6 +6,8 @@ import ( "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" + "github.com/hashicorp/terraform/terraform" + ) func TestAccServiceNetworkingConnectionCreate(t *testing.T) { @@ -29,9 +31,38 @@ func TestAccServiceNetworkingConnectionCreate(t *testing.T) { }, }, }) +} + +// Standard checkDestroy cannot be used here because destroying the network will delete +// all the networking connections so this would return false positives. +func TestAccServiceNetworkingConnectionDestroy(t *testing.T) { + t.Parallel() + + network := fmt.Sprintf("tf-test-%s", acctest.RandString(10)) + addressRange := fmt.Sprintf("tf-test-%s", acctest.RandString(10)) + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccServiceNetworkingConnection( + network, + addressRange, + "servicenetworking.googleapis.com", + ), + }, + resource.TestStep{ + Config: testAccServiceNetworkingConnectionDestroy(network, addressRange), + Check: resource.ComposeTestCheckFunc( + testServiceNetworkingConnectionDestroy("servicenetworking.googleapis.com", network, getTestProjectFromEnv()), + ), + }, + }, + }) } + func TestAccServiceNetworkingConnectionUpdate(t *testing.T) { t.Parallel() @@ -69,6 +100,29 @@ func TestAccServiceNetworkingConnectionUpdate(t *testing.T) { } +func testServiceNetworkingConnectionDestroy(parent, network, project string) resource.TestCheckFunc { + return func(s *terraform.State) error { + config := testAccProvider.Meta().(*Config) + parentService := "services/" + parent + networkName := fmt.Sprintf("projects/%s/global/networks/%s", project, network) + + + response, err := config.clientServiceNetworking.Services.Connections.List(parentService). + Network(networkName).Do() + if err != nil { + return err + } + + for _, c := range response.Connections { + if c.Network == networkName { + return fmt.Errorf("Found %s which should have been destroyed.", networkName) + } + } + + return nil + } +} + func testAccServiceNetworkingConnection(networkName, addressRangeName, serviceName string) string { return fmt.Sprintf(` resource "google_compute_network" "foobar" { @@ -90,3 +144,19 @@ resource "google_service_networking_connection" "foobar" { } `, networkName, addressRangeName, serviceName) } + +func testAccServiceNetworkingConnectionDestroy(networkName, addressRangeName string) string { + return fmt.Sprintf(` +resource "google_compute_network" "foobar" { + name = "%s" +} + +resource "google_compute_global_address" "foobar" { + name = "%s" + purpose = "VPC_PEERING" + address_type = "INTERNAL" + prefix_length = 16 + network = "${google_compute_network.foobar.self_link}" +} +`, networkName, addressRangeName) +} From 3c0a2be9d8deed13ec5a9468ec294082442230e8 Mon Sep 17 00:00:00 2001 From: Chris Stephens Date: Wed, 10 Jul 2019 20:37:17 -0700 Subject: [PATCH 3/4] removed bad imports --- third_party/terraform/utils/service_networking_operation.go | 1 - 1 file changed, 1 deletion(-) diff --git a/third_party/terraform/utils/service_networking_operation.go b/third_party/terraform/utils/service_networking_operation.go index 59a8c22f9f51..bb8d4a6ebec2 100644 --- a/third_party/terraform/utils/service_networking_operation.go +++ b/third_party/terraform/utils/service_networking_operation.go @@ -1,7 +1,6 @@ package google import ( - "github.com/hashicorp/terraform/helper/resource" "google.golang.org/api/servicenetworking/v1" ) From 6d993bae5469ec7369891d0b175b8a63c8e6a360 Mon Sep 17 00:00:00 2001 From: Chris Stephens Date: Thu, 11 Jul 2019 16:20:15 -0700 Subject: [PATCH 4/4] code review feedback --- .../resources/resource_service_networking_connection.go | 2 +- .../tests/resource_service_networking_connection_test.go | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/third_party/terraform/resources/resource_service_networking_connection.go b/third_party/terraform/resources/resource_service_networking_connection.go index b634774532e2..33c6899f9827 100644 --- a/third_party/terraform/resources/resource_service_networking_connection.go +++ b/third_party/terraform/resources/resource_service_networking_connection.go @@ -179,7 +179,7 @@ func resourceServiceNetworkingConnectionDelete(d *schema.ResourceData, meta inte res, err := sendRequestWithTimeout(config, "POST", url, obj, d.Timeout(schema.TimeoutUpdate)) if err != nil { - return fmt.Errorf("Error removing connection from network %s: %s", d.Id(), err) + return handleNotFoundError(err, d, fmt.Sprintf("ServiceNetworkingConnection %q", d.Id())) } project, err := getProject(d, config) diff --git a/third_party/terraform/tests/resource_service_networking_connection_test.go b/third_party/terraform/tests/resource_service_networking_connection_test.go index 774c89d11b45..3eafb0653fb7 100644 --- a/third_party/terraform/tests/resource_service_networking_connection_test.go +++ b/third_party/terraform/tests/resource_service_networking_connection_test.go @@ -7,7 +7,6 @@ import ( "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" - ) func TestAccServiceNetworkingConnectionCreate(t *testing.T) { @@ -62,7 +61,6 @@ func TestAccServiceNetworkingConnectionDestroy(t *testing.T) { }) } - func TestAccServiceNetworkingConnectionUpdate(t *testing.T) { t.Parallel() @@ -106,7 +104,6 @@ func testServiceNetworkingConnectionDestroy(parent, network, project string) res parentService := "services/" + parent networkName := fmt.Sprintf("projects/%s/global/networks/%s", project, network) - response, err := config.clientServiceNetworking.Services.Connections.List(parentService). Network(networkName).Do() if err != nil {