provider/google: Cloud router resource #12411

drebes · 2017-03-03T14:35:45Z

This adds support for Cloud Router, which allows BGP announcement of routes through Cloud VPN tunnels.

It's still missing documentation, I'll provide that once the code is reviewed.

It adds the following resources:

google_compute_router
google_compute_router_interface
google_compute_router_peer

It also modifies google_compute_vpn_tunnel to support Cloud Router.

A full test is provided on resource_compute_router_peer_test.go

$ make testacc TEST=./builtin/providers/google TESTARGS='-run=TestAccComputeRouter'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/03 15:38:06 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccComputeRouter -timeout 120m
=== RUN   TestAccComputeRouterInterface_import
--- PASS: TestAccComputeRouterInterface_import (197.04s)
=== RUN   TestAccComputeRouterPeer_import
--- PASS: TestAccComputeRouterPeer_import (218.85s)
=== RUN   TestAccComputeRouter_import
--- PASS: TestAccComputeRouter_import (83.30s)
=== RUN   TestAccComputeRouterInterface_basic
--- PASS: TestAccComputeRouterInterface_basic (378.11s)
=== RUN   TestAccComputeRouterPeer_basic
--- PASS: TestAccComputeRouterPeer_basic (410.42s)
=== RUN   TestAccComputeRouter_basic
--- PASS: TestAccComputeRouter_basic (96.71s)
=== RUN   TestAccComputeRouter_noRegion
--- PASS: TestAccComputeRouter_noRegion (62.47s)
=== RUN   TestAccComputeRouter_networkLink
--- PASS: TestAccComputeRouter_networkLink (84.34s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/google	1531.337s

paddycarver

Hey @drebes, thanks for opening this PR! Sorry for the long wait on the review. A PR this size is pretty intimidating and takes a lot of time to review. In the future, a PR that tackled maybe one of these resources would probably get a faster review on it, just as a heads up.

I know there are a lot of comments, but a lot of them are repetitive and are pretty minor. You did a great job at the design level, and I don't see much here that would require a refactor or major shift in how you're doing things. You get bonus points for supporting import right out of the gate! I was hoping to see Exists support, as well, but I won't hold this up over it.

That being said, it'd also be great if you could add some docs for the new resources/update the docs for the existing resource you've modified so people know how to use this great new functionality. If you need an example of that, let me know.

I know the amount of comments here is intimidating, but I think you're super close to having this be merge-ready; just some find-and-replaces and a few tweaks away.

Thanks for all the effort! If you've moved on/don't have time anymore to push this over the finish line, let me know, and I'll tackle it. :)

paddycarver · 2017-04-27T20:50:58Z

builtin/providers/google/provider.go

@@ -5,12 +5,17 @@ import (
 	"fmt"
 	"strings"

+	"github.com/hashicorp/terraform/helper/mutexkv"
+	//"github.com/hashicorp/terraform/helper/pathorcontents"


Would love to remove commented out code.

paddycarver · 2017-04-27T20:58:35Z

builtin/providers/google/resource_compute_router.go

+	if err != nil {
+		return err
+	}
+	routersService := compute.NewRoutersService(config.clientCompute)


Is there a benefit to doing this instead of using config.clientCompute.Routers?

paddycarver · 2017-04-27T21:03:40Z

builtin/providers/google/resource_compute_router.go

+		return fmt.Errorf("Error Inserting Router %s into network %s: %s", name, network, err)
+	}
+
+	err = computeOperationWaitRegion(config, op, project, region, "Inserting Router")


I'd love to see a call to d.SetId prior to the computeOperationWaitRegion, which just helps Terraform recover in case the operation times out or Terraform crashes, or something. Keep in mind this means that if there's an error inserting, d.SetId("") should be called, to clear out the ID. See an example in google_compute_instance.

paddycarver · 2017-04-27T21:04:20Z

builtin/providers/google/resource_compute_router.go

+	}
+
+	name := d.Get("name").(string)
+	routersService := compute.NewRoutersService(config.clientCompute)


Same here, I think we want config.clientCompute.Routers, just to be consistent, unless there's a difference or benefit I'm missing.

paddycarver · 2017-04-27T21:04:56Z

builtin/providers/google/resource_compute_router.go

+
+	if err != nil {
+		if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 {
+			log.Printf("[WARN] Removing Router %q because it's gone", d.Get("name").(string))


Are routers unique in name, or only unique per-region? If the latter, I think we would want to log the region here, too, right? Just so the router can be uniquely identified?

paddycarver · 2017-04-28T00:19:59Z

builtin/providers/google/resource_compute_router_test.go

+}
+`, acctest.RandString(10), acctest.RandString(10), acctest.RandString(10))
+
+var testAccComputeRouter_noRegion = fmt.Sprintf(`


Would love to see this as a function instead of a variable.

paddycarver · 2017-04-28T00:20:45Z

builtin/providers/google/resource_compute_router_test.go

+}
+`, acctest.RandString(10), acctest.RandString(10), acctest.RandString(10))
+
+var testAccComputeRouter_networkLink = fmt.Sprintf(`


Would love to see this as a function instead of a variable.

paddycarver · 2017-04-28T00:32:50Z

builtin/providers/google/resource_compute_vpn_tunnel.go

@@ -77,6 +78,7 @@ func resourceComputeVpnTunnel() *schema.Resource {
 				Type:     schema.TypeSet,
 				Optional: true,
 				ForceNew: true,
+				Computed: true,


For posterity, why does this need to be computed?

When a tunnel is created attached to a router, it will have a default policy (local and remote traffic selectors) of "0.0.0.0/0". We already had local_traffic_selector as computed, but would not get a default remote_traffic_selector from the server until now. With router based tunnels, we get them, so the need for the Computed.

paddycarver · 2017-04-28T00:34:36Z

builtin/providers/google/resource_compute_vpn_tunnel_test.go

@@ -154,6 +173,67 @@ resource "google_compute_vpn_tunnel" "foobar" {
 	acctest.RandString(10), acctest.RandString(10), acctest.RandString(10),
 	acctest.RandString(10), acctest.RandString(10))

+var testAccComputeVpnTunnel_router = fmt.Sprintf(`


Would be great to turn this into a function.

paddycarver · 2017-04-28T00:35:27Z

builtin/providers/google/resource_compute_vpn_tunnel_test.go

+	target = "${google_compute_vpn_gateway.foobar.self_link}"
+}
+resource "google_compute_router" "foobar"{
+	name = "tunnel-test-router"


Doesn't this have to be unique? If so, for resiliency, I'd like to have it randomized so we don't have collisions if the test runner fails and leaves dangling resources.

drebes · 2017-04-28T12:23:45Z

Hi, thanks for the feedback! I'll need some time to go through all the comments but will definitely do it.

paddycarver · 2017-04-28T16:44:38Z

Awesome, thanks! Feel free to tag off or ask for help or clarification if we can assist somehow. :)

drebes · 2017-05-01T15:15:09Z

Hi @paddycarver , I've implemented most of the changes, and replied to the comments where just clarification was needed.

Sorry for the large PR, creating a router without peers and interfaces is not functional so I thought it made more sense to send all as a single PR. Will send smaller, incremental ones in the future.

What did you mean by "Exists" support? Can you point me to an example?

I still need to add some docs, will work on it from now.

- config.clientCompute.Routers - peer fields renamed - more consistent logging - better handling of SetId for error handling - function for router locks - test configs as functions - simplify exists logic - use getProject, getRegion logic on acceptance tests - CheckDestroy for peers an interfaces - dynamic router name for tunnel test - extra fields for BgpPeer - resource documentation

drebes · 2017-05-03T19:35:39Z

Done with the docs, just missing feedback on the points replied above.

paddycarver · 2017-05-03T23:00:40Z

Resolved merge conflicts, will review now. :)

paddycarver

OK, this sat open in my browser for literally weeks, accruing comments slowly over time. Sorry @drebes! Only a handful more minor comments. If you want, I'm happy to schedule a short chunk of time to sit and pair with you on this and we'll get it pushed over the barrier and merged. If you're (understandably!) tired of this PR, I'm happy to carry the torch, if you'd like.

For me, the last big open question re: design is why the mutexes are being used.

paddycarver · 2017-05-03T23:06:57Z

builtin/providers/google/import_compute_router_interface_test.go

+	udp4500Rule := fmt.Sprintf("router-interface-import-test-%s", acctest.RandString(10))
+	router := fmt.Sprintf("router-interface-import-test-%s", acctest.RandString(10))
+	tunnel := fmt.Sprintf("router-interface-import-test-%s", acctest.RandString(10))
+	iface := fmt.Sprintf("router-interface-import-test-%s", acctest.RandString(10))


Do all of these need to be controlled on a per-test basis? If not, it may be clearer to just put them in the function, instead of passing them in as args. Sorry if my last review wasn't super clear; generally, my rule of thumb is if the test would want to control the value, it should be an arg, otherwise it should be defined in the function. The important part is that we're not using fmt.Sprintf as a templating function (you're not, here), and that each test case gets a unique value for things that require uniqueness. That can be achieved by just moving the variable declaration into the function, or moving the fmt.Sprintf into the function (but it has to be inside a function, not just one big fmt.Sprintf call assigned to a variable, otherwise it only gets defined once).

paddycarver · 2017-05-03T23:07:07Z

builtin/providers/google/import_compute_router_peer_test.go

+	router := fmt.Sprintf("router-peer-import-test-%s", acctest.RandString(10))
+	tunnel := fmt.Sprintf("router-peer-import-test-%s", acctest.RandString(10))
+	iface := fmt.Sprintf("router-peer-import-test-%s", acctest.RandString(10))
+	peer := fmt.Sprintf("router-peer-import-test-%s", acctest.RandString(10))


Same thing here.

paddycarver · 2017-05-03T23:07:29Z

builtin/providers/google/import_compute_router_test.go

+	resourceName := "google_compute_router.foobar"
+	network := fmt.Sprintf("router-import-test-%s", acctest.RandString(10))
+	subnet := fmt.Sprintf("router-import-test-%s", acctest.RandString(10))
+	router := fmt.Sprintf("router-import-test-%s", acctest.RandString(10))


This one, on the other hand, feels reasonable and appropriate.

paddycarver · 2017-05-03T23:08:45Z

builtin/providers/google/provider.go

 	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/hashicorp/terraform/terraform"
 	"google.golang.org/api/compute/v1"
 	"google.golang.org/api/googleapi"
 )

+// Global MutexKV
+var mutexKV = mutexkv.NewMutexKV()


I'm still unclear, sorry, why we're using a mutex. Could you explain? I see that it seems to restrict it so only one API call for the same router (or its interfaces or peers) can happen at once, but why? Is that an API limitation? Could we comment the code with why this is necessary?

Interfaces and Peers are not first class objects in the API, but attributes of the routers. As they don't have their own create/delete methods, but instead are created/destroyed with updates to the router that hosts them, the mutex is to ensure that no concurrent access on the router due to different peers/interfaces breaks the read -> add/remove -> write create/delete logic.

BTW, that's the same reason I test deletion of peers/interfaces by using a second test step with similar config. (with the omitted resource, but same testID). If I just tested destroy, this would delete the hosted router, which of course would delete the peer/interface, but not test the proper deletion of the peer/interface only.

Makes sense, thanks!

Worth noting that this isn't entirely safe--if people are modifying things outside of Terraform, the read/modify/write loop would still overwrite the changes. But our stance is people shouldn't be doing that, so I'm fine accepting this limitation.

paddycarver · 2017-05-03T23:10:39Z

builtin/providers/google/provider_test.go

+		return config.Project, nil
+	}
+	return "", fmt.Errorf("%q: required field is not set", "project")
+}


paddycarver · 2017-05-17T23:26:14Z

builtin/providers/google/resource_compute_router_peer.go

+			d.Set("peer_ip_address", peer.PeerIpAddress)
+			d.Set("peer_asn", peer.PeerAsn)
+			d.Set("advertised_route_priority", peer.AdvertisedRoutePriority)
+			d.Set("ip_address", peer.IpAddress)


Would love to see project and region persisted here, in case they change in the provider config.

paddycarver · 2017-05-17T23:27:44Z

builtin/providers/google/resource_compute_router_peer.go

+func resourceComputeRouterPeerImportState(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
+	parts := strings.Split(d.Id(), "/")
+	if len(parts) != 3 {
+		return nil, fmt.Errorf("Invalid router specifier. Expecting {region}/{router}")


I think I'm missing something. Why are we expecting three parts (line 281) if we're expecting only a single / as input? Where in that expecting string is the "name" portion that's being set in state below?

paddycarver · 2017-05-17T23:28:23Z

builtin/providers/google/resource_compute_router_peer_test.go

+	router := fmt.Sprintf("router-peer-test-%s", acctest.RandString(10))
+	tunnel := fmt.Sprintf("router-peer-test-%s", acctest.RandString(10))
+	iface := fmt.Sprintf("router-peer-test-%s", acctest.RandString(10))
+	peer := fmt.Sprintf("router-peer-test-%s", acctest.RandString(10))


Same thing here.

paddycarver · 2017-05-17T23:31:26Z

builtin/providers/google/resource_compute_vpn_tunnel.go

@@ -77,6 +78,7 @@ func resourceComputeVpnTunnel() *schema.Resource {
 				Type:     schema.TypeSet,
 				Optional: true,
 				ForceNew: true,
+				Computed: true,


paddycarver · 2017-05-17T23:31:52Z

builtin/providers/google/resource_compute_vpn_tunnel_test.go

+	udp500Rule := fmt.Sprintf("router-interface-test-%s", acctest.RandString(10))
+	udp4500Rule := fmt.Sprintf("router-interface-test-%s", acctest.RandString(10))
+	router := fmt.Sprintf("router-interface-test-%s", acctest.RandString(10))
+	tunnel := fmt.Sprintf("router-interface-test-%s", acctest.RandString(10))


Same thing here.

paddycarver · 2017-05-17T23:38:32Z

What did you mean by "Exists" support? Can you point me to an example?

We have an Exists lifecycle method (like Read, Update, Destroy) that runs before some of the other methods, allowing Terraform to check if a resource with the given ID exists or not.

// Exists is a function that is called to check if a resource still
// exists. If this returns false, then this will affect the diff
// accordingly. If this function isn't set, it will not be called. It
// is highly recommended to set it. The *ResourceData passed to Exists
// should not be modified.

(from https://godoc.org/github.com/hashicorp/terraform/helper/schema#Resource)

An example of it being used is here:

terraform/builtin/providers/google/resource_container_node_pool.go

Lines 167 to 191 in 2a36e75

    
           func resourceContainerNodePoolExists(d *schema.ResourceData, meta interface{}) (bool, error) { 
        
           	config := meta.(*Config) 
        
           	project, err := getProject(d, config) 
        
           	if err != nil { 
        
           		return false, err 
        
           	} 
        
           	zone := d.Get("zone").(string) 
        
           	name := d.Get("name").(string) 
        
           	cluster := d.Get("cluster").(string) 
        
           	_, err = config.clientContainer.Projects.Zones.Clusters.NodePools.Get( 
        
           		project, zone, cluster, name).Do() 
        
           	if err != nil { 
        
           		if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 404 { 
        
           			log.Printf("[WARN] Removing Container NodePool %q because it's gone", name) 
        
           			// The resource doesn't exist anymore 
        
           			return false, err 
        
           		} 
        
           		// There was some other error in reading the resource 
        
           		return true, err 
        
           	} 
        
           	return true, nil 
        
           }

It's not necessary, but is nice to have. At this point in the review of this resource, I'd be happy to accept it without Exists, and iterate Exists into... existence later. But if you feel the urge, there's the info. :)

Thanks for your patience and your hard work on this.

- test arguments - set region, project in state - fix import error messages - get rid of peerFound - linkDiffSuppress

drebes · 2017-05-19T21:22:30Z

Thanks for the clarification on the Exists support. Still not clear to me is when the function is called during the resource lifecycle, but I can check that. If that would give me reasonable confidence that I can get rid of the multiple checks for existence I do on read/create/delete, I see the value in adding it.

The other changes should be implemented, and the mutex question I've answered above.

paddycarver

LGTM, tests pass. Merging!

Thanks @drebes, for your Herculean patience and endurance on this one. This is great new functionality to add, and I'm excited for it!

paddycarver · 2017-05-25T03:38:06Z

builtin/providers/google/provider.go

 	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/hashicorp/terraform/terraform"
 	"google.golang.org/api/compute/v1"
 	"google.golang.org/api/googleapi"
 )

+// Global MutexKV
+var mutexKV = mutexkv.NewMutexKV()


Makes sense, thanks!

Worth noting that this isn't entirely safe--if people are modifying things outside of Terraform, the read/modify/write loop would still overwrite the changes. But our stance is people shouldn't be doing that, so I'm fine accepting this limitation.

ghost · 2020-04-12T01:58:08Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

drebes force-pushed the cloud_router branch from 822ce31 to d4f92a9 Compare March 3, 2017 15:22

paddycarver added new-resource provider/google-cloud labels Mar 3, 2017

drebes force-pushed the cloud_router branch 2 times, most recently from 95318ad to 05b2f10 Compare March 25, 2017 05:21

drebes force-pushed the cloud_router branch 2 times, most recently from 641aceb to cc38e07 Compare April 26, 2017 19:32

providers/google: cloud_router

55d5f99

drebes force-pushed the cloud_router branch from cc38e07 to 55d5f99 Compare April 26, 2017 19:32

paddycarver suggested changes Apr 28, 2017

View reviewed changes

drebes force-pushed the cloud_router branch 6 times, most recently from e0b682b to 97ef205 Compare May 1, 2017 14:35

drebes force-pushed the cloud_router branch 4 times, most recently from 7a7bfaa to 9968166 Compare May 3, 2017 19:15

drebes force-pushed the cloud_router branch from 9968166 to 4dddca3 Compare May 3, 2017 19:22

Merge branch 'master' into cloud_router

8e69d75

Merge branch 'master' into cloud_router

50f8b94

paddycarver suggested changes May 17, 2017

View reviewed changes

drebes force-pushed the cloud_router branch 2 times, most recently from 7f48364 to 5785f15 Compare May 19, 2017 20:57

wip: review changes:

6ab9d82

- test arguments - set region, project in state - fix import error messages - get rid of peerFound - linkDiffSuppress

drebes force-pushed the cloud_router branch from 5785f15 to 6ab9d82 Compare May 19, 2017 21:06

paddycarver approved these changes May 25, 2017

View reviewed changes

paddycarver merged commit 63ce0da into hashicorp:master May 25, 2017

cblecker mentioned this pull request May 26, 2017

Support for Google Cloud Router for VPN #7520

Closed

ghost locked and limited conversation to collaborators Apr 12, 2020

provider/google: Cloud router resource #12411

provider/google: Cloud router resource #12411

Conversation

drebes commented Mar 3, 2017 • edited Loading

paddycarver 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drebes commented Apr 28, 2017

paddycarver commented Apr 28, 2017

drebes commented May 1, 2017

drebes commented May 3, 2017

paddycarver commented May 3, 2017

paddycarver 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

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

paddycarver commented May 17, 2017

drebes commented May 19, 2017

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 12, 2020

drebes commented Mar 3, 2017 •

edited

Loading