From 97850d88edc345231ae025d89712eebe97f889c1 Mon Sep 17 00:00:00 2001 From: Vincent Roseberry Date: Tue, 3 Oct 2017 13:24:02 -0700 Subject: [PATCH] Firewall uses v1 API if the priority is unset or has the default value. (#500) * api_versions supports default value * Firewall use v1 API if the priority is set to default value (1000) --- google/api_versions.go | 49 ++++++++++++++++----- google/api_versions_test.go | 66 ++++++++++++++++++++++++++--- google/resource_compute_firewall.go | 2 +- 3 files changed, 100 insertions(+), 17 deletions(-) diff --git a/google/api_versions.go b/google/api_versions.go index a594b44ef85..ccea4722fa4 100644 --- a/google/api_versions.go +++ b/google/api_versions.go @@ -45,7 +45,7 @@ type TerraformResourceData interface { func getComputeApiVersion(d TerraformResourceData, resourceVersion ComputeApiVersion, features []Feature) ComputeApiVersion { versions := map[ComputeApiVersion]struct{}{resourceVersion: struct{}{}} for _, feature := range features { - if feature.InUseBy(d) { + if feature.InUseByDefault(d) { versions[feature.Version] = struct{}{} } } @@ -58,8 +58,12 @@ func getComputeApiVersion(d TerraformResourceData, resourceVersion ComputeApiVer // version, to determine what version of the API is required in order to update the resource. func getComputeApiVersionUpdate(d TerraformResourceData, resourceVersion ComputeApiVersion, features, updateOnlyFields []Feature) ComputeApiVersion { versions := map[ComputeApiVersion]struct{}{resourceVersion: struct{}{}} - schemaVersion := getComputeApiVersion(d, resourceVersion, features) - versions[schemaVersion] = struct{}{} + + for _, feature := range features { + if feature.InUseByUpdate(d) { + versions[feature.Version] = struct{}{} + } + } for _, feature := range updateOnlyFields { if feature.HasChangeBy(d) { @@ -83,6 +87,10 @@ type Feature struct { // // Note: beta field nested inside a SET are NOT supported at the moment. Item string + + // Optional, only set if your field has a default value. + // If the value for the field is equal to the DefaultValue, we assume the beta feature is not activated. + DefaultValue interface{} } // Returns true when a feature has been modified. @@ -93,16 +101,37 @@ func (s Feature) HasChangeBy(d TerraformResourceData) bool { return d.HasChange(s.Item) } -// Return true when a feature appears in schema or has been modified. -func (s Feature) InUseBy(d TerraformResourceData) bool { - return inUseBy(d, s.Item) +type InUseFunc func(d TerraformResourceData, path string, defaultValue interface{}) bool + +func defaultInUseFunc(d TerraformResourceData, path string, defaultValue interface{}) bool { + // At read and delete time, there is no change. + // At create time, all fields are marked has changed. We should only consider the feature active if the field has + // a value set and that this value is not the default value. + value, ok := d.GetOk(path) + return ok && value != defaultValue +} + +func updateInUseFunc(d TerraformResourceData, path string, defaultValue interface{}) bool { + // During a resource update, if the beta field has changes, the feature is considered active even if the new value + // is the default value. This is because the beta API must be called to change the value of the field back to the + // default value. + value, ok := d.GetOk(path) + return (ok && value != defaultValue) || d.HasChange(path) +} + +// Return true when a feature appears in schema and doesn't hold the default value. +func (s Feature) InUseByDefault(d TerraformResourceData) bool { + return inUseBy(d, s.Item, s.DefaultValue, defaultInUseFunc) +} + +func (s Feature) InUseByUpdate(d TerraformResourceData) bool { + return inUseBy(d, s.Item, s.DefaultValue, updateInUseFunc) } -func inUseBy(d TerraformResourceData, path string) bool { +func inUseBy(d TerraformResourceData, path string, defaultValue interface{}, inUseFunc InUseFunc) bool { pos := strings.Index(path, "*") if pos == -1 { - _, ok := d.GetOk(path) - return ok || d.HasChange(path) + return inUseFunc(d, path, defaultValue) } prefix := path[0:pos] @@ -117,7 +146,7 @@ func inUseBy(d TerraformResourceData, path string) bool { count := v.(int) for i := 0; i < count; i++ { nestedPath := fmt.Sprintf("%s%d%s", prefix, i, suffix) - if inUseBy(d, nestedPath) { + if inUseBy(d, nestedPath, defaultValue, inUseFunc) { return true } } diff --git a/google/api_versions_test.go b/google/api_versions_test.go index 4f537b4d748..fc9039f7bbe 100644 --- a/google/api_versions_test.go +++ b/google/api_versions_test.go @@ -19,7 +19,7 @@ func TestComputeApiVersion(t *testing.T) { UpdateOnlyFields []Feature ExpectedApiVersions }{ - "no beta fields": { + "no beta field": { FieldsInSchema: map[string]interface{}{ "normal_field": "foo", }, @@ -29,7 +29,7 @@ func TestComputeApiVersion(t *testing.T) { Update: baseVersion, }, }, - "beta fields no set": { + "beta field not set": { Features: []Feature{{Version: betaVersion, Item: "beta_field"}}, FieldsInSchema: map[string]interface{}{ "normal_field": "foo", @@ -40,7 +40,7 @@ func TestComputeApiVersion(t *testing.T) { Update: baseVersion, }, }, - "beta fields set": { + "beta field set": { Features: []Feature{{Version: betaVersion, Item: "beta_field"}}, FieldsInSchema: map[string]interface{}{ "normal_field": "foo", @@ -52,7 +52,7 @@ func TestComputeApiVersion(t *testing.T) { Update: betaVersion, }, }, - "update only beta fields": { + "update only beta field": { FieldsInSchema: map[string]interface{}{ "normal_field": "foo", }, @@ -64,7 +64,7 @@ func TestComputeApiVersion(t *testing.T) { Update: betaVersion, }, }, - "nested beta fields not set": { + "nested beta field not set": { Features: []Feature{{Version: betaVersion, Item: "list_field.*.beta_nested_field"}}, FieldsInSchema: map[string]interface{}{ "list_field.#": 2, @@ -77,7 +77,7 @@ func TestComputeApiVersion(t *testing.T) { Update: baseVersion, }, }, - "nested beta fields set": { + "nested beta field set": { Features: []Feature{{Version: betaVersion, Item: "list_field.*.beta_nested_field"}}, FieldsInSchema: map[string]interface{}{ "list_field.#": 2, @@ -104,6 +104,60 @@ func TestComputeApiVersion(t *testing.T) { Update: betaVersion, }, }, + "beta field has default value": { + Features: []Feature{{Version: betaVersion, Item: "beta_field", DefaultValue: "bar"}}, + FieldsInSchema: map[string]interface{}{ + "normal_field": "foo", + "beta_field": "bar", + }, + ExpectedApiVersions: ExpectedApiVersions{ + Create: baseVersion, + ReadDelete: baseVersion, + Update: baseVersion, + }, + }, + "beta field is updated to default value": { + Features: []Feature{{Version: betaVersion, Item: "beta_field", DefaultValue: "bar"}}, + FieldsInSchema: map[string]interface{}{ + "normal_field": "foo", + "beta_field": "bar", + }, + UpdatedFields: []string{"beta_field"}, + ExpectedApiVersions: ExpectedApiVersions{ + Create: baseVersion, + ReadDelete: baseVersion, + Update: betaVersion, + }, + }, + "nested beta field has default value": { + Features: []Feature{{Version: betaVersion, Item: "list_field.*.beta_nested_field", DefaultValue: "baz"}}, + 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", + }, + ExpectedApiVersions: ExpectedApiVersions{ + Create: baseVersion, + ReadDelete: baseVersion, + Update: baseVersion, + }, + }, + "nested beta field is updated default value": { + Features: []Feature{{Version: betaVersion, Item: "list_field.*.beta_nested_field", DefaultValue: "baz"}}, + 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", + }, + UpdatedFields: []string{"list_field.1.beta_nested_field"}, + ExpectedApiVersions: ExpectedApiVersions{ + Create: baseVersion, + ReadDelete: baseVersion, + Update: betaVersion, + }, + }, } for tn, tc := range cases { diff --git a/google/resource_compute_firewall.go b/google/resource_compute_firewall.go index 7fc23e6d3b4..13d79c81b94 100644 --- a/google/resource_compute_firewall.go +++ b/google/resource_compute_firewall.go @@ -20,7 +20,7 @@ var FirewallVersionedFeatures = []Feature{ Feature{Version: v0beta, Item: "deny"}, Feature{Version: v0beta, Item: "direction"}, Feature{Version: v0beta, Item: "destination_ranges"}, - Feature{Version: v0beta, Item: "priority"}, + Feature{Version: v0beta, Item: "priority", DefaultValue: COMPUTE_FIREWALL_PRIORITY_DEFAULT}, } func resourceComputeFirewall() *schema.Resource {