Skip to content

Commit

Permalink
Firewall network field now supports self_link in addition of name (#477)
Browse files Browse the repository at this point in the history
  • Loading branch information
rosbo authored Sep 28, 2017
1 parent 39a8588 commit bc25c02
Show file tree
Hide file tree
Showing 5 changed files with 116 additions and 15 deletions.
40 changes: 40 additions & 0 deletions google/field_helpers.go
Original file line number Diff line number Diff line change
@@ -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)
}
36 changes: 36 additions & 0 deletions google/field_helpers_test.go
Original file line number Diff line number Diff line change
@@ -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())
}
}
}
19 changes: 6 additions & 13 deletions google/resource_compute_firewall.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bytes"
"fmt"
"sort"
"strings"

"github.com/hashicorp/terraform/helper/hashcode"
"github.com/hashicorp/terraform/helper/schema"
Expand Down Expand Up @@ -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": {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
34 changes: 33 additions & 1 deletion google/resource_compute_firewall_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -27,6 +28,7 @@ func TestAccComputeFirewall_basic(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckComputeFirewallExists(
"google_compute_firewall.foobar", &firewall),
testAccCheckComputeFirewallApiVersion(&firewall),
),
},
},
Expand All @@ -48,6 +50,7 @@ func TestAccComputeFirewall_update(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckComputeFirewallExists(
"google_compute_firewall.foobar", &firewall),
testAccCheckComputeFirewallApiVersion(&firewall),
),
},
resource.TestStep{
Expand All @@ -57,6 +60,7 @@ func TestAccComputeFirewall_update(t *testing.T) {
"google_compute_firewall.foobar", &firewall),
testAccCheckComputeFirewallPorts(
&firewall, "80-255"),
testAccCheckComputeFirewallApiVersion(&firewall),
),
},
},
Expand All @@ -78,6 +82,7 @@ func TestAccComputeFirewall_priority(t *testing.T) {
testAccCheckComputeBetaFirewallExists(
"google_compute_firewall.foobar", &firewall),
testAccCheckComputeFirewallHasPriority(&firewall, 1001),
testAccCheckComputeFirewallBetaApiVersion(&firewall),
),
}},
})
Expand All @@ -98,6 +103,7 @@ func TestAccComputeFirewall_noSource(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckComputeFirewallExists(
"google_compute_firewall.foobar", &firewall),
testAccCheckComputeFirewallApiVersion(&firewall),
),
},
},
Expand All @@ -119,6 +125,7 @@ func TestAccComputeFirewall_denied(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckComputeBetaFirewallExists("google_compute_firewall.foobar", &firewall),
testAccCheckComputeBetaFirewallDenyPorts(&firewall, "22"),
testAccCheckComputeFirewallBetaApiVersion(&firewall),
),
},
},
Expand All @@ -140,6 +147,7 @@ func TestAccComputeFirewall_egress(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckComputeBetaFirewallExists("google_compute_firewall.foobar", &firewall),
testAccCheckComputeBetaFirewallEgress(&firewall),
testAccCheckComputeFirewallBetaApiVersion(&firewall),
),
},
},
Expand Down Expand Up @@ -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" {
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion website/docs/r/compute_firewall.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit bc25c02

Please sign in to comment.