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.

Sign up for GitHub

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

Add support for alias_ip_range in google_compute_instance network interface #375

Merged
merged 6 commits into from
Sep 7, 2017
Merged

Add support for alias_ip_range in google_compute_instance network interface #375

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2076f16
Add support for alias_ip_range in google_compute_instance network int…
rosbo Aug 30, 2017
e1eb2a6
Generic method to diffsuppress self_link or name field.
rosbo Sep 6, 2017
bb44d58
Add documentation on field path for api versions
rosbo Sep 6, 2017
0f0f2c6
make fmt
rosbo Sep 6, 2017
4f7ab64
Improve documentation
rosbo Sep 7, 2017
838cfeb
Improve documentation
rosbo Sep 7, 2017
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
32 changes: 30 additions & 2 deletions google/api_versions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package google

import (
"encoding/json"
"fmt"
"strings"
)

type ComputeApiVersion uint8
Expand Down Expand Up @@ -84,8 +86,34 @@ func (s Feature) HasChangeBy(d TerraformResourceData) bool {

// Return true when a feature appears in schema or has been modified.
func (s Feature) InUseBy(d TerraformResourceData) bool {
_, ok := d.GetOk(s.Item)
return ok || s.HasChangeBy(d)
return inUseBy(d, s.Item)
}

func inUseBy(d TerraformResourceData, path string) bool {
pos := strings.Index(path, "*")
Copy link
Contributor

Choose a reason for hiding this comment

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

I have to say I'm not a huge fan of this syntax. I appreciate that you do have to make some sort of change however to be able to see if this feature is in use for any of the network_interface properties.

If I see a '*' I would expect it to be globbing like a regex rather than having it match a sublist. Anyway to make the behavior like that?

(Alternatively, if we could see if a feature is in use by using a more structural syntax I'd totally love that (similar to how we navigate the schema.ResourceData struct)

Also, can we document this syntax in the inUseBy method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no use case for globbing on a field. I don't expect why a beta field would be present under two different field. inUseBy is only use for api versions.

You can see the * as globbing on the indices. Usually, you would have field.5.nested_field.

I am not sure I follow your suggestion about structural syntax...

I will add more documentation once we agree on the syntax.

Copy link
Contributor

@selmanj selmanj Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

But you're globbing on a field right now! (or at least, you're globbing on an integer)

I guess I'm saying is that if we introduce syntax like '*' but it is specific to integers, it's asking for a bug in the future when someone uses '*' like globbing. Given that globbing would work here, would it be too hard to add?

For a more structural syntax, I meant being able to receive and parse the actual schema.ResourceData struct using GetOk and other syntax to see if a given feature is in use, rather than using the serialize string data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supporting globbing also on field names would definitely add complexity (and potentially bugs) when there is clearly no needs for this. However, I can make sure it fails if with a nice error message if someone uses the * on a field name instead of an indice. This way, we prevent somebody from misusing it without knowing they are misusing it.

Copy link
Contributor

@selmanj selmanj Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't want to block this whole PR on the syntax (especially since what's currently-implemented would work if * was a globbing symbol) but I'm starting to worry that this inUseBy method is an anti-pattern. Right now it depends on the serialized state and not the provided schema.ResourceData which provides a well-defined set of methods for dealing with the schema. My understanding is that inUseBy is used because it's convenient, but if we ever depend on values like optionals or validation then we'll regret using it here.

That said, we can probably have that discussion outside of this PR. If we document the '*' syntax I'm ok as-is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused - inUseBy uses the methods from schema.ResourceData; TerraformResourceData is just an interface for the two methods inUseBy needs, since we don't need the rest of the behaviour of the class and are able to unit test this code w/ a mock of that interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, after looking at the code, you were right and I was confused; it IS operating on schema.ResourceData; I had no idea you could directly access attributes like foobar.# to get the size of a list for example using the Get or GetOk method. For some reason I assumed it was using something similar to the the attributes map that we constantly assert on in unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documentation added.

if pos == -1 {
_, ok := d.GetOk(path)
return ok || d.HasChange(path)
}

prefix := path[0:pos]
suffix := path[pos+1:]

v, ok := d.GetOk(prefix + "#")

if !ok {
return false
}

count := v.(int)
for i := 0; i < count; i++ {
nestedPath := fmt.Sprintf("%s%d%s", prefix, i, suffix)
if inUseBy(d, nestedPath) {
return true
}
}

return false
}

func maxVersion(versionsInUse map[ComputeApiVersion]struct{}) ComputeApiVersion {
Expand Down
95 changes: 84 additions & 11 deletions google/api_versions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import "testing"

func TestResourceWithOnlyBaseVersionFields(t *testing.T) {
d := &ResourceDataMock{
FieldsInSchema: []string{"normal_field"},
FieldsInSchema: map[string]interface{}{
"normal_field": "foo",
},
}

resourceVersion := v1
Expand All @@ -22,7 +24,10 @@ func TestResourceWithOnlyBaseVersionFields(t *testing.T) {
func TestResourceWithBetaFields(t *testing.T) {
resourceVersion := v1
d := &ResourceDataMock{
FieldsInSchema: []string{"normal_field", "beta_field"},
FieldsInSchema: map[string]interface{}{
"normal_field": "foo",
"beta_field": "bar",
},
}

expectedVersion := v0beta
Expand All @@ -40,7 +45,9 @@ func TestResourceWithBetaFields(t *testing.T) {
func TestResourceWithBetaFieldsNotInSchema(t *testing.T) {
resourceVersion := v1
d := &ResourceDataMock{
FieldsInSchema: []string{"normal_field"},
FieldsInSchema: map[string]interface{}{
"normal_field": "foo",
},
}

expectedVersion := v1
Expand All @@ -58,7 +65,10 @@ func TestResourceWithBetaFieldsNotInSchema(t *testing.T) {
func TestResourceWithBetaUpdateFields(t *testing.T) {
resourceVersion := v1
d := &ResourceDataMock{
FieldsInSchema: []string{"normal_field", "beta_update_field"},
FieldsInSchema: map[string]interface{}{
"normal_field": "foo",
"beta_field": "bar",
},
FieldsWithHasChange: []string{"beta_update_field"},
}

Expand All @@ -73,11 +83,76 @@ func TestResourceWithBetaUpdateFields(t *testing.T) {
if computeApiVersion != expectedVersion {
t.Errorf("Expected to see version: %v. Saw version: %v.", expectedVersion, computeApiVersion)
}
}

func TestResourceWithOnlyBaseNestedFields(t *testing.T) {
resourceVersion := v1
d := &ResourceDataMock{
FieldsInSchema: map[string]interface{}{
"list_field.#": 2,
"list_field.0.normal_field": "foo",
"list_field.1.normal_field": "bar",
},
}

computeApiVersion := getComputeApiVersion(d, resourceVersion, []Feature{})
if computeApiVersion != resourceVersion {
t.Errorf("Expected to see version: %v. Saw version: %v.", resourceVersion, computeApiVersion)
}

computeApiVersion = getComputeApiVersionUpdate(d, resourceVersion, []Feature{}, []Feature{{Version: resourceVersion, Item: "list_field.*.beta_nested_field"}})
if computeApiVersion != resourceVersion {
t.Errorf("Expected to see version: %v. Saw version: %v.", resourceVersion, computeApiVersion)
}
}

func TestResourceWithBetaNestedFields(t *testing.T) {
resourceVersion := v1
d := &ResourceDataMock{
FieldsInSchema: map[string]interface{}{
"list_field.#": 2,
"list_field.0.normal_field": "foo",
"list_field.1.normal_field": "bar",
"list_field.1.beta_nested_field": "baz",
},
}

expectedVersion := v0beta
computeApiVersion := getComputeApiVersion(d, resourceVersion, []Feature{{Version: expectedVersion, Item: "list_field.*.beta_nested_field"}})
if computeApiVersion != expectedVersion {
t.Errorf("Expected to see version: %v. Saw version: %v.", expectedVersion, computeApiVersion)
}

computeApiVersion = getComputeApiVersionUpdate(d, resourceVersion, []Feature{{Version: expectedVersion, Item: "list_field.*.beta_nested_field"}}, []Feature{})
if computeApiVersion != expectedVersion {
t.Errorf("Expected to see version: %v. Saw version: %v.", expectedVersion, computeApiVersion)
}
}

func TestResourceWithBetaDoubleNestedFields(t *testing.T) {
resourceVersion := v1
d := &ResourceDataMock{
FieldsInSchema: map[string]interface{}{
"list_field.#": 1,
"list_field.0.nested_list_field.#": 1,
"list_field.0.nested_list_field.0.beta_nested_field": "foo",
},
}

expectedVersion := v0beta
computeApiVersion := getComputeApiVersion(d, resourceVersion, []Feature{{Version: expectedVersion, Item: "list_field.*.nested_list_field.*.beta_nested_field"}})
if computeApiVersion != expectedVersion {
t.Errorf("Expected to see version: %v. Saw version: %v.", expectedVersion, computeApiVersion)
}

computeApiVersion = getComputeApiVersionUpdate(d, resourceVersion, []Feature{{Version: expectedVersion, Item: "list_field.*.nested_list_field.*.beta_nested_field"}}, []Feature{})
if computeApiVersion != expectedVersion {
t.Errorf("Expected to see version: %v. Saw version: %v.", expectedVersion, computeApiVersion)
}
}

type ResourceDataMock struct {
FieldsInSchema []string
FieldsInSchema map[string]interface{}
FieldsWithHasChange []string
}

Expand All @@ -93,13 +168,11 @@ func (d *ResourceDataMock) HasChange(key string) bool {
}

func (d *ResourceDataMock) GetOk(key string) (interface{}, bool) {
exists := false
for _, val := range d.FieldsInSchema {
if key == val {
exists = true
for k, v := range d.FieldsInSchema {
if key == k {
return v, true
}

}

return nil, exists
return nil, false
}
52 changes: 51 additions & 1 deletion google/resource_compute_instance.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ var InstanceVersionedFeatures = []Feature{
Version: v0beta,
Item: "min_cpu_platform",
},
{
Version: v0beta,
Item: "network_interface.*.alias_ip_range",
},
}

func stringScopeHashcode(v interface{}) int {
Expand Down Expand Up @@ -337,6 +341,27 @@ func resourceComputeInstance() *schema.Resource {
},
},
},

"alias_ip_range": &schema.Schema{
Type: schema.TypeList,
Optional: true,
ForceNew: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"ip_cidr_range": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: true,
DiffSuppressFunc: ipCidrRangeDiffSuppress,
},
"subnetwork_range_name": &schema.Schema{
Type: schema.TypeString,
Optional: true,
ForceNew: true,
},
},
},
},
},
},
},
Expand Down Expand Up @@ -793,6 +818,7 @@ func resourceComputeInstanceCreate(d *schema.ResourceData, meta interface{}) err
iface.Network = networkLink
iface.Subnetwork = subnetworkLink
iface.NetworkIP = address
iface.AliasIpRanges = expandAliasIpRanges(d.Get(prefix + ".alias_ip_range").([]interface{}))

// Handle access_config structs
accessConfigsCount := d.Get(prefix + ".access_config.#").(int)
Expand Down Expand Up @@ -1032,9 +1058,10 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error
"name": iface.Name,
"address": iface.NetworkIP,
"network": iface.Network,
"subnetwork": iface.Subnetwork,
"subnetwork": ConvertSelfLinkToV1(iface.Subnetwork),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the DiffSuppressFunc not covering this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that subnetwork can be specified using the name or the self-link.

If a user specify a name in the tf config, the diff is suppressed correctly using linkDiffSuppress.
However, if the user specify a self-link in the tf config, we get a diff because one link is beta and the other one is v1 and linkDiffSuppress doesn't work for that case.

The other option is to change the DiffSuppressFunc to something along these lines:

DiffSuppressFunc: func(...) bool { 
   return compareSelfLinkRelativePaths(...) || linkDiffSuppress(...)
}

We could factored this logic into its own method compareRegionalSelfLinkOrResourceName or something like that. The same way it was done for compareGlobalSelfLinkOrResourceName

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense, thanks!

Looking at how linkDiffSuppress is used for both scopes, would we be able to generalise compareGlobalSelfLinkOrResourceName, and use that instead?

As I understand it, we are always going to store a full self link in state, so old will always be a full link; we can then split new on /, and check its length, where there are two cases:

  • newParts has a length of 1: The user has specified a resource name, so we should split old into oldParts and check the last element of each.
  • newParts has any other length: The user has not specified a name, and so they are using a relative self link (starting from projects/), or a full self link; we can compare by using compareSelfLinkRelativePaths on old and new.

That way we can cover both cases with the same diff function, and this should work for Zonal resources too if there is a similar case, without adding a third function for that as well.

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

"subnetwork_project": getProjectFromSubnetworkLink(iface.Subnetwork),
"access_config": accessConfigs,
"alias_ip_range": flattenAliasIpRange(iface.AliasIpRanges),
})
}
}
Expand Down Expand Up @@ -1573,6 +1600,18 @@ func expandGuestAccelerators(zone string, configs []interface{}) []*computeBeta.
return guestAccelerators
}

func expandAliasIpRanges(ranges []interface{}) []*computeBeta.AliasIpRange {
ipRanges := make([]*computeBeta.AliasIpRange, 0, len(ranges))
for _, raw := range ranges {
data := raw.(map[string]interface{})
ipRanges = append(ipRanges, &computeBeta.AliasIpRange{
IpCidrRange: data["ip_cidr_range"].(string),
SubnetworkRangeName: data["subnetwork_range_name"].(string),
})
}
return ipRanges
}

func flattenGuestAccelerators(zone string, accelerators []*computeBeta.AcceleratorConfig) []map[string]interface{} {
acceleratorsSchema := make([]map[string]interface{}, 0, len(accelerators))
for _, accelerator := range accelerators {
Expand Down Expand Up @@ -1605,6 +1644,17 @@ func flattenBetaScheduling(scheduling *computeBeta.Scheduling) []map[string]inte
return result
}

func flattenAliasIpRange(ranges []*computeBeta.AliasIpRange) []map[string]interface{} {
rangesSchema := make([]map[string]interface{}, 0, len(ranges))
for _, ipRange := range ranges {
rangesSchema = append(rangesSchema, map[string]interface{}{
"ip_cidr_range": ipRange.IpCidrRange,
"subnetwork_range_name": ipRange.SubnetworkRangeName,
})
}
return rangesSchema
}

func getProjectFromSubnetworkLink(subnetwork string) string {
r := regexp.MustCompile(SubnetworkLinkRegex)
if !r.MatchString(subnetwork) {
Expand Down
Loading