Add peering feature to google_compute_network. #242

rosbo · 2017-07-25T22:34:12Z

issue #178

I added a few comments on my own PR to ask for the opinions of the reviewers on a few things

rosbo · 2017-07-25T22:35:17Z

google/resource_compute_network.go

+							Type:             schema.TypeString,
+							Required:         true,
+							ValidateFunc:     validateRegexp(peerNetworkLinkRegex),
+							DiffSuppressFunc: peerNetworkLinkDiffSuppress,


I could achieve the same thing by using a StateFunc that always saves the value to a canonical format. Any preferences?

I have no preferences here personally. I think that we could migrate between the two at will without even using a MigrateFunc if we ever wanted to, too.

rosbo · 2017-07-25T23:00:10Z

google/resource_compute_network.go

 )

+const peerNetworkLinkRegex = "projects/(" + projectRegex + ")/global/networks/((?:[a-z](?:[-a-z0-9]*[a-z0-9])?))$"


For the peer network, full URL and partial URL are supported:

https://www.googleapis.com/compute/v1/projects/my-project/global/networks/network-1 OK

projects/my-project/global/networks/network-1 OK

global/networks/network-1 ACCEPTED by GCP but REJECTED by my implementation

network-1 WRONG

The reason why I am rejecting (3) even though GCP considers it valid is because I can't properly detect changes if I don't require the project in the URL. GCP, if the project is missing from the URL uses the default project.

If I have this configuration:

resource "google_compute_network" "default" { name = "default" peering { name = "foo" network = "global/networks/other-network" } }

After the first terraform apply, the network is created and the peering.0.network field containshttps://www.googleapis.com/compute/v1/projects/my-project/global/networks/other-network (this value is read from the GCP and GCP always return a full url.

If I call terraform plan. I should not see a diff. However, I have the following two strings for the network field:

old https://www.googleapis.com/compute/v1/projects/my-project/global/networks/other-network

new global/networks/other-network
I could suppress the diff by only comparing the network name. However, this would introduce a new bug, if I change the network from /my-project/global/networks/other-network to ANOTHER-PROJECT/global/networks/other-network, I would detect the diff because both network have the same name.

If I try using the StateFunc, I don't have access to the current project in a StateFunc in order to transform global/networks/other-network to its canonical form.

Anybody have a solution for this?
Or are we fine with not supporting the "project-less" version. This is a new field so we are not breaking anybody. We are only forcing them to be more explicit from the start.

Personally, I'm OK with only accepting the first two forms. Terraform self links will always be using full or relative partial URLs, and while omitting the project is a nice shorthand, it's information that users will always have.

I'd be interested if we could file/bump an issue against Terraform itself for getting more information inside a StateFunc/DiffSuppressFunc; if we knew the project in either, we could allow the shorthand as well.

rileykarson

Overall looks good! I want to make it more clear in tests/the example in the docs that peering requires both networks to opt-in, and have a few minor nits otherwise.

rileykarson · 2017-07-26T15:54:15Z

google/validation.go

@@ -6,6 +6,8 @@ import (
 	"regexp"
 )

+const projectRegex = "(?:(?:[-a-z0-9]{1,63}\\.)*(?:[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?):)?(?:[0-9]{1,19}|(?:[a-z0-9](?:[-a-z0-9]{0,61}[a-z0-9])?))"


nit: Let's use a capital P here as if this variable was exported. We are in the same google package as the rest of the provider so it's not a necessary change, but imo it makes it more clear that the value comes from another file when used.

rileykarson · 2017-07-26T15:55:01Z

website/docs/r/compute_network.html.markdown

+
+The `peering` block supports:
+
+* `name` - (Required) Name of the peering.


nit: Add a new line between each attribute.

rileykarson · 2017-07-26T15:57:29Z

google/resource_compute_network_test.go

@@ -71,6 +71,38 @@ func TestAccComputeNetwork_custom_subnet(t *testing.T) {
 	})
 }

+func TestAccComputeNetwork_peering(t *testing.T) {


Both of these test cases handle one network requesting a peering with the other, but these networks won't connect since each network has to define the peering. Can we add a test that successfully sets up a peered network?

We'll probably need to define subnetworks with different CIDR rules to do that instead of automatically creating them, since their IP ranges can't overlap; I connected two networks in the Cloud Console using 192.168.0.0/24 and 10.0.0.0/9 if I remember right.

rileykarson · 2017-07-26T16:01:41Z

google/resource_compute_network.go

+
+	d.Partial(true)
+
+	if d.HasChange("peering") {


We have a lot of code dedicated to working with changing a set of peerings, but our tests only have 1 or 0 on each network. Let's test that code by adding a test that has several peerings, and updates keeping some but removing/adding others.

Right now, since I am changing the name of the peering, it does one add and one remove. I will add more coverage though. One problem is that I can't add more than 3 networks because by default, GCD account have a default network and a cap of 5 on the maximum number of networks. I like to leave at least one open to be able to allow some level of manual testing while my test run.

rileykarson · 2017-07-26T16:07:57Z

google/resource_compute_network.go

+	return add, remove
+}
+
+func addPeering(config *Config, project, network string, add []map[string]interface{}) error {


s/addPeering/addPeerings

rileykarson · 2017-07-26T16:11:07Z

google/resource_compute_network.go

+		old, new := d.GetChange("peering")
+		add, remove := calcAddRemoveNetworkPeerings(old, new)
+
+		if len(remove) > 0 {


Let's factor this out into removePeerings as well, to mirror addPeering

rileykarson · 2017-07-26T16:17:00Z

google/resource_compute_network.go

 )

+const peerNetworkLinkRegex = "projects/(" + projectRegex + ")/global/networks/((?:[a-z](?:[-a-z0-9]*[a-z0-9])?))$"


Personally, I'm OK with only accepting the first two forms. Terraform self links will always be using full or relative partial URLs, and while omitting the project is a nice shorthand, it's information that users will always have.

I'd be interested if we could file/bump an issue against Terraform itself for getting more information inside a StateFunc/DiffSuppressFunc; if we knew the project in either, we could allow the shorthand as well.

rileykarson · 2017-07-26T16:18:19Z

google/resource_compute_network.go

+							Type:             schema.TypeString,
+							Required:         true,
+							ValidateFunc:     validateRegexp(peerNetworkLinkRegex),
+							DiffSuppressFunc: peerNetworkLinkDiffSuppress,


I have no preferences here personally. I think that we could migrate between the two at will without even using a MigrateFunc if we ever wanted to, too.

rileykarson · 2017-07-26T16:19:42Z

website/docs/r/compute_network.html.markdown

@@ -17,6 +17,16 @@ resource "google_compute_network" "default" {
  name                    = "foobar"
  auto_create_subnetworks = "true"
 }
+
+resource "google_compute_network" "other" {


Same as with the tests - let's make this example a valid network peering.

If we can have one network automatically create subnetworks and we manually define them for another & manage to peer successfully, we'd be able to show even more of how to use this resource to readers.

rileykarson · 2017-07-26T16:21:02Z

website/docs/r/compute_network.html.markdown

+* `network` - (Required) Resource link of the peer network.
+* `auto_create_routes` - (Optional) If set to true, the routes between the two networks will
+  be created and managed automatically.
+* `state` - (Computed) State for the peering.


Output-only attributes typically live under Attributes Reference, but I'm not sure exactly what it should look like because these are part of the peering block.

Yeah, when I wrote these I wasn't sure where to put it because it is part of the peering block. @danawillow any preference?

rosbo · 2017-07-26T21:49:59Z

After thinking more about it and also discussing with Riley, it may makes more sense to have the vpc peering feature be its own resource.

One problem comes with circular dependency when creating peer network:

resource "google_compute_network" "network1" {
  name = "network-1" 

  auto_create_subnetworks = false

  peering {
    name = "one-two-peering"
    # Error: cycle network1 depends on network2 and vice versa
    network = "${google_compute_network.network2.self_link}"
  } 
}

resource "google_compute_network" "network2" {
  name = "network-2"  

  auto_create_subnetworks = false

  peering {
    name = "one-two-peering"
    # Error: cycle network1 depends on network2 and vice versa
    network = "${google_compute_network.network1.self_link}"
  }
}

To go around this a user must set manually (w/o interpolations) the self link:

peering {
    name = "one-two-peering"
    network = "https://www.googleapis.com/compute/v1/projects/rosbo-personal/global/networks/network-2"
 }

If we create a separate resource for peering. The config would look like:

resource "google_compute_network" "network1" {
  name = "network-1" 
  auto_create_subnetworks = false
}

resource "google_compute_network" "network2" {
  name = "network-2" 
  auto_create_subnetworks = false
}

resource "google_compute_network_peering" "peering1-2" {
   name = "one-two-peering"
   network = "${google_compute_network.network1.self_link}"
   peer_network = "${google_compute_network.network2.self_link}"
}

resource "google_compute_network_peering" "peering2-1" {
   name = "one-two-peering"
   network = "${google_compute_network.network2.self_link}"
   peer_network = "${google_compute_network.network1.self_link}"
}

And alternative for the google_compute_network_peering resource would be to support bidirectionality. This way, we remove the need for creating two peering resources.

resource "google_compute_network_peering" "peering" {
   name = "one-two-peering"
   network = "${google_compute_network.network2.self_link}"
   peer_network = "${google_compute_network.network1.self_link}"
   bidirectional = true  # Defaults to false. False means only a peering from `network` to `peer_network` is created.
}

@danawillow Any thoughts?

rosbo · 2017-07-27T22:54:43Z

Closed in favor of #259 using more fine grained resources.

…p#242)

ghost · 2020-03-31T14:08:32Z

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

Add peering feature to google_compute_network.

6da50df

rosbo requested review from danawillow and rileykarson July 25, 2017 22:34

rosbo commented Jul 25, 2017

View reviewed changes

grubernaut added the enhancement label Jul 26, 2017

rileykarson requested changes Jul 26, 2017

View reviewed changes

rosbo mentioned this pull request Jul 27, 2017

Add new resource google_compute_network_peering #259

Merged

rosbo closed this Jul 27, 2017

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

Remove duplicates from provider resource map, add dupe test (hashicor…

ac12a34

…p#242)

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add peering feature to google_compute_network. #242

Add peering feature to google_compute_network. #242

rosbo commented Jul 25, 2017 •

edited

Loading

rosbo Jul 25, 2017

rileykarson Jul 26, 2017

rosbo Jul 25, 2017

rileykarson Jul 26, 2017

rileykarson left a comment

rileykarson Jul 26, 2017

rileykarson Jul 26, 2017

rileykarson Jul 26, 2017

rileykarson Jul 26, 2017

rosbo Jul 26, 2017

rileykarson Jul 26, 2017

rileykarson Jul 26, 2017

rileykarson Jul 26, 2017

rileykarson Jul 26, 2017

rileykarson Jul 26, 2017

rileykarson Jul 26, 2017

rosbo Jul 26, 2017

rosbo commented Jul 26, 2017 •

edited

Loading

rosbo commented Jul 27, 2017

ghost commented Mar 31, 2020

		)

		const peerNetworkLinkRegex = "projects/(" + projectRegex + ")/global/networks/((?:[a-z](?:[-a-z0-9]*[a-z0-9])?))$"


		The `peering` block supports:

		* `name` - (Required) Name of the peering.

Add peering feature to google_compute_network. #242

Add peering feature to google_compute_network. #242

Conversation

rosbo commented Jul 25, 2017 • edited Loading

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

rileykarson 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

rosbo commented Jul 26, 2017 • edited Loading

rosbo commented Jul 27, 2017

ghost commented Mar 31, 2020

rosbo commented Jul 25, 2017 •

edited

Loading

rosbo commented Jul 26, 2017 •

edited

Loading