From 5f52aba3aeefd92ebeb9256aad1744d1d3e9ed3f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Apr 2019 09:34:39 -0400 Subject: [PATCH 1/3] Remove unknown value strings from apply diffs The synthetic config value used to create the Apply diff should contain no unknown config values. Any remaining UnknownConfigValues were due to that being used as a placeholder for values yet to be computed, and these should be marked NewComputed in the diff. --- helper/schema/shims.go | 25 ++++++++++++++++ helper/schema/shims_test.go | 60 +++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/helper/schema/shims.go b/helper/schema/shims.go index 4099d82d4..e66fc09d4 100644 --- a/helper/schema/shims.go +++ b/helper/schema/shims.go @@ -6,6 +6,7 @@ import ( "github.com/zclconf/go-cty/cty" ctyjson "github.com/zclconf/go-cty/cty/json" + "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/terraform" ) @@ -31,6 +32,8 @@ func diffFromValues(prior, planned cty.Value, res *Resource, cust CustomizeDiffF configSchema := res.CoreConfigSchema() cfg := terraform.NewResourceConfigShimmed(planned, configSchema) + removeConfigUnknowns(cfg.Config) + removeConfigUnknowns(cfg.Raw) diff, err := schemaMap(res.Schema).Diff(instanceState, cfg, cust, nil, false) if err != nil { @@ -40,6 +43,28 @@ func diffFromValues(prior, planned cty.Value, res *Resource, cust CustomizeDiffF return diff, err } +// During apply the only unknown values are those which are to be computed by +// the resource itself. These may have been marked as unknown config values, and +// need to be removed to prevent the UnknownVariableValue from appearing the diff. +func removeConfigUnknowns(cfg map[string]interface{}) { + for k, v := range cfg { + switch v := v.(type) { + case string: + if v == config.UnknownVariableValue { + cfg[k] = "" + } + case []interface{}: + for _, i := range v { + if m, ok := i.(map[string]interface{}); ok { + removeConfigUnknowns(m) + } + } + case map[string]interface{}: + removeConfigUnknowns(v) + } + } +} + // ApplyDiff takes a cty.Value state and applies a terraform.InstanceDiff to // get a new cty.Value state. This is used to convert the diff returned from // the legacy provider Diff method to the state required for the new diff --git a/helper/schema/shims_test.go b/helper/schema/shims_test.go index e3e9df745..7c523f42b 100644 --- a/helper/schema/shims_test.go +++ b/helper/schema/shims_test.go @@ -3590,6 +3590,14 @@ func TestShimSchemaMap_Diff(t *testing.T) { } } + // there would be no unknown config variables during apply, so + // return early here. + for _, v := range tc.ConfigVariables { + if s, ok := v.Value.(string); ok && s == config.UnknownVariableValue { + return + } + } + // our diff function can't set DestroyTainted, but match the // expected value here for the test fixtures if tainted { @@ -3610,3 +3618,55 @@ func TestShimSchemaMap_Diff(t *testing.T) { func resourceSchemaToBlock(s map[string]*Schema) *configschema.Block { return (&Resource{Schema: s}).CoreConfigSchema() } + +func TestRemoveConfigUnknowns(t *testing.T) { + cfg := map[string]interface{}{ + "id": "74D93920-ED26-11E3-AC10-0800200C9A66", + "route_rules": []interface{}{ + map[string]interface{}{ + "cidr_block": "74D93920-ED26-11E3-AC10-0800200C9A66", + "destination": "0.0.0.0/0", + "destination_type": "CIDR_BLOCK", + "network_entity_id": "1", + }, + map[string]interface{}{ + "cidr_block": "74D93920-ED26-11E3-AC10-0800200C9A66", + "destination": "0.0.0.0/0", + "destination_type": "CIDR_BLOCK", + "sub_block": []interface{}{ + map[string]interface{}{ + "computed": "74D93920-ED26-11E3-AC10-0800200C9A66", + }, + }, + }, + }, + } + + expect := map[string]interface{}{ + "id": "", + "route_rules": []interface{}{ + map[string]interface{}{ + "cidr_block": "", + "destination": "0.0.0.0/0", + "destination_type": "CIDR_BLOCK", + "network_entity_id": "1", + }, + map[string]interface{}{ + "cidr_block": "", + "destination": "0.0.0.0/0", + "destination_type": "CIDR_BLOCK", + "sub_block": []interface{}{ + map[string]interface{}{ + "computed": "", + }, + }, + }, + }, + } + + removeConfigUnknowns(cfg) + + if !reflect.DeepEqual(cfg, expect) { + t.Fatalf("\nexpected: %#v\ngot: %#v", expect, cfg) + } +} From af8115dc9b0c265aac8b7cde011e8480659fdd29 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Apr 2019 09:39:45 -0400 Subject: [PATCH 2/3] removing the ~ set flag is no longer needed The computed set sigil ~ should no longer appear in the diffs, because the config will be cleaned before generating the diff. --- helper/plugin/grpc_provider.go | 31 +++++++------------------------ 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go index f0e6af3e6..143e0f645 100644 --- a/helper/plugin/grpc_provider.go +++ b/helper/plugin/grpc_provider.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "strconv" - "strings" "github.com/zclconf/go-cty/cty" ctyconvert "github.com/zclconf/go-cty/cty/convert" @@ -777,17 +776,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A } } - // We need to fix any sets that may be using the "~" index prefix to - // indicate partially computed. The special sigil isn't really used except - // as a clue to visually indicate that the set isn't wholly known. - for k, d := range diff.Attributes { - if strings.Contains(k, ".~") { - delete(diff.Attributes, k) - k = strings.Replace(k, ".~", ".", -1) - diff.Attributes[k] = d - } - } - // add NewExtra Fields that may have been stored in the private data if newExtra := private[newExtraKey]; newExtra != nil { for k, v := range newExtra.(map[string]interface{}) { @@ -806,16 +794,14 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A diff.Meta = private } - // We need to turn off any RequiresNew. There could be attributes - // without changes in here inserted by helper/schema, but if they have - // RequiresNew then the state will will be dropped from the ResourceData. - for k := range diff.Attributes { - diff.Attributes[k].RequiresNew = false - } - - // check that any "removed" attributes actually exist in the prior state, or - // helper/schema will confuse itself for k, d := range diff.Attributes { + // We need to turn off any RequiresNew. There could be attributes + // without changes in here inserted by helper/schema, but if they have + // RequiresNew then the state will be dropped from the ResourceData. + d.RequiresNew = false + + // Check that any "removed" attributes that don't actually exist in the + // prior state, or helper/schema will confuse itself if d.NewRemoved { if _, ok := priorState.Attributes[k]; !ok { delete(diff.Attributes, k) @@ -845,9 +831,6 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A return resp, nil } - // here we use the planned state to check for unknown/zero containers values - // when normalizing the flatmap. - // We keep the null val if we destroyed the resource, otherwise build the // entire object, even if the new state was nil. newStateVal, err = schema.StateValueFromInstanceState(newInstanceState, blockForShimming.ImpliedType()) From 8d32229f7d776f56b30fc306328cb3db422055f4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 10 Apr 2019 09:41:11 -0400 Subject: [PATCH 3/3] add test fetching computed set value by address This is not a recommended method, but it does serve to verify that the set values in the ResourceData internal state are correctly computed, which indicates that the expected configuration was passed in. --- builtin/providers/test/resource_state_func.go | 44 +++++++++++++++++++ .../test/resource_state_func_test.go | 25 +++++++++++ 2 files changed, 69 insertions(+) diff --git a/builtin/providers/test/resource_state_func.go b/builtin/providers/test/resource_state_func.go index 46c4b9b20..609e5ea53 100644 --- a/builtin/providers/test/resource_state_func.go +++ b/builtin/providers/test/resource_state_func.go @@ -6,6 +6,7 @@ import ( "fmt" "math/rand" + "github.com/hashicorp/terraform/helper/hashcode" "github.com/hashicorp/terraform/helper/schema" ) @@ -35,6 +36,26 @@ func testResourceStateFunc() *schema.Resource { Type: schema.TypeString, Optional: true, }, + + // set block with computed elements + "set_block": { + Type: schema.TypeSet, + Optional: true, + Set: setBlockHash, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "required": { + Type: schema.TypeString, + Required: true, + }, + "optional": { + Type: schema.TypeString, + Optional: true, + Computed: true, + }, + }, + }, + }, }, } } @@ -44,6 +65,13 @@ func stateFuncHash(v interface{}) string { return hex.EncodeToString(hash[:]) } +func setBlockHash(v interface{}) int { + m := v.(map[string]interface{}) + required, _ := m["required"].(string) + optional, _ := m["optional"].(string) + return hashcode.String(fmt.Sprintf("%s|%s", required, optional)) +} + func testResourceStateFuncCreate(d *schema.ResourceData, meta interface{}) error { d.SetId(fmt.Sprintf("%x", rand.Int63())) @@ -57,6 +85,22 @@ func testResourceStateFuncCreate(d *schema.ResourceData, meta interface{}) error } } + // Check that we can lookup set elements by our computed hash. + // This is not advised, but we can use this to make sure the final diff was + // prepared with the correct values. + setBlock, ok := d.GetOk("set_block") + if ok { + set := setBlock.(*schema.Set) + for _, obj := range set.List() { + idx := setBlockHash(obj) + requiredAddr := fmt.Sprintf("%s.%d.%s", "set_block", idx, "required") + _, ok := d.GetOkExists(requiredAddr) + if !ok { + return fmt.Errorf("failed to get attr %q from %#v", fmt.Sprintf(requiredAddr), d.State().Attributes) + } + } + } + return testResourceStateFuncRead(d, meta) } diff --git a/builtin/providers/test/resource_state_func_test.go b/builtin/providers/test/resource_state_func_test.go index afdbd924e..cf5726eea 100644 --- a/builtin/providers/test/resource_state_func_test.go +++ b/builtin/providers/test/resource_state_func_test.go @@ -58,3 +58,28 @@ resource "test_resource_state_func" "foo" { }, }) } + +func TestResourceStateFunc_getOkSetElem(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_state_func" "foo" { +} + +resource "test_resource_state_func" "bar" { + set_block { + required = "foo" + optional = test_resource_state_func.foo.id + } + set_block { + required = test_resource_state_func.foo.id + } +} + `), + }, + }, + }) +}