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 4 commits
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
37 changes: 34 additions & 3 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 @@ -71,7 +73,10 @@ func getComputeApiVersionUpdate(d TerraformResourceData, resourceVersion Compute
// A field of a resource and the version of the Compute API required to use it.
type Feature struct {
Version ComputeApiVersion
Item string
// Path to the beta field. Supports:
// - beta field: "min_cpu_platform"
// - nested beta field: "network_interface.*.alias_ip_range"
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; this is just a bit too terse for me (and maybe i"m wrong but I don't think it works for all nested fields right? Only integer-nested fields?)

Maybe something like:

 // - integer-nested field: If the Item field contains a '*', then the feature is considered to be in-use if it is present in any element of the list or set (e.g. "network_interface.*.alias_ip_range" is considered to be in-use if "network_interface.3.alias_ip_range" was set in the state

Hopefully you can come up with something better (also I like your name path better for the field name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the documentation incorporating some of the bits you suggested. Hopefully it is clearer now.

Yes, it works for all nested fields (not just integer nested fields). The proof is that alias_ip_range is of TypeList and network_interface is of TypeList.

It doesn't support beta field nested under a set at the moment. It would add complexity and be slow since we would need to scan the whole state to find the elements in the set (we can't rely on zero-indexed incremental indices like we do for lists). If we ever need to support this, we can implement this efficiently with a trie.

Note: I will rename the Item for Path or FieldPath in a separate PR. Since updating the field touches a lot of files, I prefer to do it in a separate PR that can get merged quickly to avoid the rebase/merge hassle and also to avoid polluting this already long PR .

Item string
}

// Returns true when a feature has been modified.
Expand All @@ -84,8 +89,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
}
54 changes: 52 additions & 2 deletions 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 @@ -290,15 +294,15 @@ func resourceComputeInstance() *schema.Resource {
Optional: true,
Computed: true,
ForceNew: true,
DiffSuppressFunc: linkDiffSuppress,
DiffSuppressFunc: compareSelfLinkOrResourceName,
},

"subnetwork": &schema.Schema{
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
DiffSuppressFunc: linkDiffSuppress,
DiffSuppressFunc: compareSelfLinkOrResourceName,
},

"subnetwork_project": &schema.Schema{
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 @@ -1035,6 +1061,7 @@ func resourceComputeInstanceRead(d *schema.ResourceData, meta interface{}) error
"subnetwork": iface.Subnetwork,
"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