Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firewall network field now supports self_link in addition of name #477

Merged
merged 2 commits into from
Sep 28, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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(fmt.Sprintf(networkLinkTemplate, "(.+)", "(.+)"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me quite uncomfortable; something about printing regex chunks into another regex made me squirm a bit

How do you feel about just inlining the regex here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Done


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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brainstorming outloud here; none of these are action items:

I wonder if there's a way we can eventually merge all the url parsing code; unfortunately it doesn't seem obvious how since each has their own path scheme. Right now the code is relatively short and simple so the cost of having it isn't much.

Maybe at some point we should combine them all into a single namespace? urltools or resourcetools or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. I think once we have created a few of these FieldValue classes, we will see patterns emerging and we can create common utils to make it easy to create these FieldValue classes.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor quibble; if the struct is small it's nice to not use a ptr here for the receiving struct; that way its obvious that your method doesn't mutate the struct its called on (there's runtime benefits as well but I don't think we should be concerned with that)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, you eliminated an API call with this change :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. And the other good thing is that sometimes, this call was failing if the user didn't have the iam permission for reading the network! It was kind of unexpected since we are setting up a firewall.

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}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Other tests are referencing the network by name.

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