From bc25c02cbf4946436229a8d10b5ba7afa870633e Mon Sep 17 00:00:00 2001 From: Vincent Roseberry Date: Thu, 28 Sep 2017 12:02:39 -0700 Subject: [PATCH] Firewall network field now supports self_link in addition of name (#477) --- google/field_helpers.go | 40 +++++++++++++++++++ google/field_helpers_test.go | 36 +++++++++++++++++ google/resource_compute_firewall.go | 19 +++------ google/resource_compute_firewall_test.go | 34 +++++++++++++++- website/docs/r/compute_firewall.html.markdown | 2 +- 5 files changed, 116 insertions(+), 15 deletions(-) create mode 100644 google/field_helpers.go create mode 100644 google/field_helpers_test.go diff --git a/google/field_helpers.go b/google/field_helpers.go new file mode 100644 index 00000000..e327bf83 --- /dev/null +++ b/google/field_helpers.go @@ -0,0 +1,40 @@ +package google + +import ( + "fmt" + "regexp" +) + +const networkLinkTemplate = "projects/%s/global/networks/%s" + +var networkLinkRegex = regexp.MustCompile("projects/(.+)/global/networks/(.+)") + +type NetworkFieldValue struct { + Project string + Name string +} + +// Parses a `network` supporting 4 different formats: +// - https://www.googleapis.com/compute/{version}/projects/myproject/global/networks/my-network +// - projects/myproject/global/networks/my-network +// - global/networks/my-network (default project is used) +// - my-network (default project is used) +func ParseNetworkFieldValue(network string, config *Config) *NetworkFieldValue { + if networkLinkRegex.MatchString(network) { + parts := networkLinkRegex.FindStringSubmatch(network) + + return &NetworkFieldValue{ + Project: parts[1], + Name: parts[2], + } + } + + return &NetworkFieldValue{ + Project: config.Project, + Name: GetResourceNameFromSelfLink(network), + } +} + +func (f NetworkFieldValue) RelativeLink() string { + return fmt.Sprintf(networkLinkTemplate, f.Project, f.Name) +} diff --git a/google/field_helpers_test.go b/google/field_helpers_test.go new file mode 100644 index 00000000..62cb5ea2 --- /dev/null +++ b/google/field_helpers_test.go @@ -0,0 +1,36 @@ +package google + +import "testing" + +func TestParseNetworkFieldValue(t *testing.T) { + cases := map[string]struct { + Network string + ExpectedRelativeLink string + Config *Config + }{ + "network is a full self link": { + Network: "https://www.googleapis.com/compute/v1/projects/myproject/global/networks/my-network", + ExpectedRelativeLink: "projects/myproject/global/networks/my-network", + }, + "network is a relative self link": { + Network: "projects/myproject/global/networks/my-network", + ExpectedRelativeLink: "projects/myproject/global/networks/my-network", + }, + "network is a partial relative self link": { + Network: "global/networks/my-network", + ExpectedRelativeLink: "projects/default-project/global/networks/my-network", + Config: &Config{Project: "default-project"}, + }, + "network is the name only": { + Network: "my-network", + ExpectedRelativeLink: "projects/default-project/global/networks/my-network", + Config: &Config{Project: "default-project"}, + }, + } + + for tn, tc := range cases { + if fieldValue := ParseNetworkFieldValue(tc.Network, tc.Config); fieldValue.RelativeLink() != tc.ExpectedRelativeLink { + t.Fatalf("bad: %s, expected relative link to be '%s' but got '%s'", tn, tc.ExpectedRelativeLink, fieldValue.RelativeLink()) + } + } +} diff --git a/google/resource_compute_firewall.go b/google/resource_compute_firewall.go index fb30a52f..7fc23e6d 100644 --- a/google/resource_compute_firewall.go +++ b/google/resource_compute_firewall.go @@ -4,7 +4,6 @@ import ( "bytes" "fmt" "sort" - "strings" "github.com/hashicorp/terraform/helper/hashcode" "github.com/hashicorp/terraform/helper/schema" @@ -44,9 +43,10 @@ func resourceComputeFirewall() *schema.Resource { }, "network": { - Type: schema.TypeString, - Required: true, - ForceNew: true, + Type: schema.TypeString, + Required: true, + ForceNew: true, + DiffSuppressFunc: compareSelfLinkOrResourceName, }, "priority": { @@ -294,10 +294,9 @@ func resourceComputeFirewallRead(d *schema.ResourceData, meta interface{}) error } } - networkUrl := strings.Split(firewall.Network, "/") d.Set("self_link", ConvertSelfLinkToV1(firewall.SelfLink)) d.Set("name", firewall.Name) - d.Set("network", networkUrl[len(networkUrl)-1]) + d.Set("network", ConvertSelfLinkToV1(firewall.Network)) // Unlike most other Beta properties, direction will always have a value even when // a zero is sent by the client. We'll never revert back to v1 without conditionally reading it. @@ -405,12 +404,6 @@ func resourceComputeFirewallDelete(d *schema.ResourceData, meta interface{}) err func resourceFirewall(d *schema.ResourceData, meta interface{}, computeApiVersion ComputeApiVersion) (*computeBeta.Firewall, error) { config := meta.(*Config) - project, _ := getProject(d, config) - - network, err := config.clientCompute.Networks.Get(project, d.Get("network").(string)).Do() - if err != nil { - return nil, fmt.Errorf("Error reading network: %s", err) - } // Build up the list of allowed entries var allowed []*computeBeta.FirewallAllowed @@ -494,7 +487,7 @@ func resourceFirewall(d *schema.ResourceData, meta interface{}, computeApiVersio Name: d.Get("name").(string), Description: d.Get("description").(string), Direction: d.Get("direction").(string), - Network: network.SelfLink, + Network: ParseNetworkFieldValue(d.Get("network").(string), config).RelativeLink(), Allowed: allowed, Denied: denied, SourceRanges: sourceRanges, diff --git a/google/resource_compute_firewall_test.go b/google/resource_compute_firewall_test.go index 4c2230cd..41b77da1 100644 --- a/google/resource_compute_firewall_test.go +++ b/google/resource_compute_firewall_test.go @@ -10,6 +10,7 @@ import ( computeBeta "google.golang.org/api/compute/v0.beta" "google.golang.org/api/compute/v1" + "strings" ) func TestAccComputeFirewall_basic(t *testing.T) { @@ -27,6 +28,7 @@ func TestAccComputeFirewall_basic(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckComputeFirewallExists( "google_compute_firewall.foobar", &firewall), + testAccCheckComputeFirewallApiVersion(&firewall), ), }, }, @@ -48,6 +50,7 @@ func TestAccComputeFirewall_update(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckComputeFirewallExists( "google_compute_firewall.foobar", &firewall), + testAccCheckComputeFirewallApiVersion(&firewall), ), }, resource.TestStep{ @@ -57,6 +60,7 @@ func TestAccComputeFirewall_update(t *testing.T) { "google_compute_firewall.foobar", &firewall), testAccCheckComputeFirewallPorts( &firewall, "80-255"), + testAccCheckComputeFirewallApiVersion(&firewall), ), }, }, @@ -78,6 +82,7 @@ func TestAccComputeFirewall_priority(t *testing.T) { testAccCheckComputeBetaFirewallExists( "google_compute_firewall.foobar", &firewall), testAccCheckComputeFirewallHasPriority(&firewall, 1001), + testAccCheckComputeFirewallBetaApiVersion(&firewall), ), }}, }) @@ -98,6 +103,7 @@ func TestAccComputeFirewall_noSource(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckComputeFirewallExists( "google_compute_firewall.foobar", &firewall), + testAccCheckComputeFirewallApiVersion(&firewall), ), }, }, @@ -119,6 +125,7 @@ func TestAccComputeFirewall_denied(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckComputeBetaFirewallExists("google_compute_firewall.foobar", &firewall), testAccCheckComputeBetaFirewallDenyPorts(&firewall, "22"), + testAccCheckComputeFirewallBetaApiVersion(&firewall), ), }, }, @@ -140,6 +147,7 @@ func TestAccComputeFirewall_egress(t *testing.T) { Check: resource.ComposeTestCheckFunc( testAccCheckComputeBetaFirewallExists("google_compute_firewall.foobar", &firewall), testAccCheckComputeBetaFirewallEgress(&firewall), + testAccCheckComputeFirewallBetaApiVersion(&firewall), ), }, }, @@ -270,6 +278,30 @@ func testAccCheckComputeBetaFirewallEgress(firewall *computeBeta.Firewall) resou } } +func testAccCheckComputeFirewallBetaApiVersion(firewall *computeBeta.Firewall) resource.TestCheckFunc { + return func(s *terraform.State) error { + // The self-link of the network field is used to determine which API was used when fetching + // the state from the API. + if !strings.Contains(firewall.Network, "compute/beta") { + return fmt.Errorf("firewall beta API was not used") + } + + return nil + } +} + +func testAccCheckComputeFirewallApiVersion(firewall *compute.Firewall) resource.TestCheckFunc { + return func(s *terraform.State) error { + // The self-link of the network field is used to determine which API was used when fetching + // the state from the API. + if !strings.Contains(firewall.Network, "compute/v1") { + return fmt.Errorf("firewall beta API was not used") + } + + return nil + } +} + func testAccComputeFirewall_basic(network, firewall string) string { return fmt.Sprintf(` resource "google_compute_network" "foobar" { @@ -299,7 +331,7 @@ func testAccComputeFirewall_update(network, firewall string) string { resource "google_compute_firewall" "foobar" { name = "firewall-test-%s" description = "Resource created for Terraform acceptance testing" - network = "${google_compute_network.foobar.name}" + network = "${google_compute_network.foobar.self_link}" source_tags = ["foo"] allow { diff --git a/website/docs/r/compute_firewall.html.markdown b/website/docs/r/compute_firewall.html.markdown index 9f43b209..ae8e840d 100644 --- a/website/docs/r/compute_firewall.html.markdown +++ b/website/docs/r/compute_firewall.html.markdown @@ -40,7 +40,7 @@ The following arguments are supported: * `name` - (Required) A unique name for the resource, required by GCE. Changing this forces a new resource to be created. -* `network` - (Required) The name of the network to attach this firewall to. +* `network` - (Required) The name or self_link of the network to attach this firewall to. * `allow` - (Required) Can be specified multiple times for each allow rule. Each allow block supports fields documented below.