From 998ba6e6e183540312af6870c5941bd3bdd43400 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 6 Aug 2020 21:45:57 -0400 Subject: [PATCH 1/3] remove extra attrs found in state json While removal of attributes can be handled by providers through the UpgradeResourceState call, data sources may need to be evaluated before reading, and they have no upgrade path in the provider protocol. Strip out extra attributes during state decoding when they are no longer present in the schema, and there is no schema upgrade pending. --- terraform/eval_state_upgrade.go | 96 +++++++++++++++++ terraform/eval_state_upgrade_test.go | 148 +++++++++++++++++++++++++++ 2 files changed, 244 insertions(+) create mode 100644 terraform/eval_state_upgrade_test.go diff --git a/terraform/eval_state_upgrade.go b/terraform/eval_state_upgrade.go index acf671c68..e03349c37 100644 --- a/terraform/eval_state_upgrade.go +++ b/terraform/eval_state_upgrade.go @@ -1,6 +1,7 @@ package terraform import ( + "encoding/json" "fmt" "log" @@ -9,6 +10,7 @@ import ( "github.com/hashicorp/terraform/providers" "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/tfdiags" + "github.com/zclconf/go-cty/cty" ) // UpgradeResourceState will, if necessary, run the provider-defined upgrade @@ -19,6 +21,17 @@ import ( // If any errors occur during upgrade, error diagnostics are returned. In that // case it is not safe to proceed with using the original state object. func UpgradeResourceState(addr addrs.AbsResourceInstance, provider providers.Interface, src *states.ResourceInstanceObjectSrc, currentSchema *configschema.Block, currentVersion uint64) (*states.ResourceInstanceObjectSrc, tfdiags.Diagnostics) { + // Remove any attributes from state that are not present in the schema. + // This was previously taken care of by the provider, but data sources do + // not go through the UpgradeResourceState process. + // + // Legacy flatmap state is already taken care of during conversion. + // If the schema version is be changed, then allow the provider to handle + // removed attributes. + if len(src.AttrsJSON) > 0 && src.SchemaVersion == currentVersion { + src.AttrsJSON = stripRemovedStateAttributes(src.AttrsJSON, currentSchema.ImpliedType()) + } + if addr.Resource.Resource.Mode != addrs.ManagedResourceMode { // We only do state upgrading for managed resources. return src, nil @@ -105,3 +118,86 @@ func UpgradeResourceState(addr addrs.AbsResourceInstance, provider providers.Int } return new, diags } + +// stripRemovedStateAttributes deletes any attributes no longer present in the +// schema, so that the json can be correctly decoded. +func stripRemovedStateAttributes(state []byte, ty cty.Type) []byte { + jsonMap := map[string]interface{}{} + err := json.Unmarshal(state, &jsonMap) + if err != nil { + // we just log any errors here, and let the normal decode process catch + // invalid JSON. + log.Printf("[ERROR] UpgradeResourceState: %s", err) + return state + } + + // if no changes were made, we return the original state to ensure nothing + // was altered in the marshaling process. + if !removeRemovedAttrs(jsonMap, ty) { + return state + } + + js, err := json.Marshal(jsonMap) + if err != nil { + // if the json map was somehow mangled enough to not marhsal, something + // went horribly wrong + panic(err) + } + + return js +} + +// strip out the actual missing attributes, and return a bool indicating if any +// changes were made. +func removeRemovedAttrs(v interface{}, ty cty.Type) bool { + modified := false + // we're only concerned with finding maps that correspond to object + // attributes + switch v := v.(type) { + case []interface{}: + switch { + // If these aren't blocks the next call will be a noop + case ty.IsListType() || ty.IsSetType(): + eTy := ty.ElementType() + for _, eV := range v { + modified = removeRemovedAttrs(eV, eTy) || modified + } + } + return modified + case map[string]interface{}: + switch { + case ty.IsMapType(): + // map blocks aren't yet supported, but handle this just in case + eTy := ty.ElementType() + for _, eV := range v { + modified = removeRemovedAttrs(eV, eTy) || modified + } + return modified + + case ty == cty.DynamicPseudoType: + log.Printf("[DEBUG] UpgradeResourceState: ignoring dynamic block: %#v\n", v) + return false + + case ty.IsObjectType(): + attrTypes := ty.AttributeTypes() + for attr, attrV := range v { + attrTy, ok := attrTypes[attr] + if !ok { + log.Printf("[DEBUG] UpgradeResourceState: attribute %q no longer present in schema", attr) + delete(v, attr) + modified = true + continue + } + + modified = removeRemovedAttrs(attrV, attrTy) || modified + } + return modified + default: + // This shouldn't happen, and will fail to decode further on, so + // there's no need to handle it here. + log.Printf("[WARN] UpgradeResourceState: unexpected type %#v for map in json state", ty) + return false + } + } + return modified +} diff --git a/terraform/eval_state_upgrade_test.go b/terraform/eval_state_upgrade_test.go new file mode 100644 index 000000000..11ef77b5f --- /dev/null +++ b/terraform/eval_state_upgrade_test.go @@ -0,0 +1,148 @@ +package terraform + +import ( + "reflect" + "testing" + + "github.com/zclconf/go-cty/cty" +) + +func TestStripRemovedStateAttributes(t *testing.T) { + cases := []struct { + name string + state map[string]interface{} + expect map[string]interface{} + ty cty.Type + modified bool + }{ + { + "removed string", + map[string]interface{}{ + "a": "ok", + "b": "gone", + }, + map[string]interface{}{ + "a": "ok", + }, + cty.Object(map[string]cty.Type{ + "a": cty.String, + }), + true, + }, + { + "removed null", + map[string]interface{}{ + "a": "ok", + "b": nil, + }, + map[string]interface{}{ + "a": "ok", + }, + cty.Object(map[string]cty.Type{ + "a": cty.String, + }), + true, + }, + { + "removed nested string", + map[string]interface{}{ + "a": "ok", + "b": map[string]interface{}{ + "a": "ok", + "b": "removed", + }, + }, + map[string]interface{}{ + "a": "ok", + "b": map[string]interface{}{ + "a": "ok", + }, + }, + cty.Object(map[string]cty.Type{ + "a": cty.String, + "b": cty.Object(map[string]cty.Type{ + "a": cty.String, + }), + }), + true, + }, + { + "removed nested list", + map[string]interface{}{ + "a": "ok", + "b": map[string]interface{}{ + "a": "ok", + "b": []interface{}{"removed"}, + }, + }, + map[string]interface{}{ + "a": "ok", + "b": map[string]interface{}{ + "a": "ok", + }, + }, + cty.Object(map[string]cty.Type{ + "a": cty.String, + "b": cty.Object(map[string]cty.Type{ + "a": cty.String, + }), + }), + true, + }, + { + "removed keys in set of objs", + map[string]interface{}{ + "a": "ok", + "b": map[string]interface{}{ + "a": "ok", + "set": []interface{}{ + map[string]interface{}{ + "x": "ok", + "y": "removed", + }, + map[string]interface{}{ + "x": "ok", + "y": "removed", + }, + }, + }, + }, + map[string]interface{}{ + "a": "ok", + "b": map[string]interface{}{ + "a": "ok", + "set": []interface{}{ + map[string]interface{}{ + "x": "ok", + }, + map[string]interface{}{ + "x": "ok", + }, + }, + }, + }, + cty.Object(map[string]cty.Type{ + "a": cty.String, + "b": cty.Object(map[string]cty.Type{ + "a": cty.String, + "set": cty.Set(cty.Object(map[string]cty.Type{ + "x": cty.String, + })), + }), + }), + true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + modified := removeRemovedAttrs(tc.state, tc.ty) + if !reflect.DeepEqual(tc.state, tc.expect) { + t.Fatalf("expected: %#v\n got: %#v\n", tc.expect, tc.state) + } + if modified != tc.modified { + t.Fatal("incorrect return value") + } + }) + } +} From 3cf84bb3f979de01c9809a859cfb06ccfe2fc14c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 6 Aug 2020 22:25:46 -0400 Subject: [PATCH 2/3] don't add state to the validate context The validate command should work with the configuration, but when validate was run at the start of a plan or apply command the state was inserted in preparation for the next walk. This could lead to errors when the resource schemas had changes and the state could not be upgraded or decoded. --- backend/local/backend_local.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/backend/local/backend_local.go b/backend/local/backend_local.go index 4fffc5b66..312456207 100644 --- a/backend/local/backend_local.go +++ b/backend/local/backend_local.go @@ -72,6 +72,13 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, *configload. log.Printf("[TRACE] backend/local: retrieving local state snapshot for workspace %q", op.Workspace) opts.State = s.State() + // Prepare a separate opts and context for validation, which doesn't use + // any state ensuring that we only validate the config, since evaluation + // will automatically reference the state when available. + validateOpts := opts + validateOpts.State = nil + var validateCtx *terraform.Context + var tfCtx *terraform.Context var ctxDiags tfdiags.Diagnostics var configSnap *configload.Snapshot @@ -89,9 +96,18 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, *configload. // Write sources into the cache of the main loader so that they are // available if we need to generate diagnostic message snippets. op.ConfigLoader.ImportSourcesFromSnapshot(configSnap) + + // create a validation context with no state + validateCtx, _, _ = b.contextFromPlanFile(op.PlanFile, validateOpts, stateMeta) + // diags from here will be caught above + } else { log.Printf("[TRACE] backend/local: building context for current working directory") tfCtx, configSnap, ctxDiags = b.contextDirect(op, opts) + + // create a validation context with no state + validateCtx, _, _ = b.contextDirect(op, validateOpts) + // diags from here will be caught above } diags = diags.Append(ctxDiags) if diags.HasErrors() { @@ -117,7 +133,7 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, *configload. // If validation is enabled, validate if b.OpValidation { log.Printf("[TRACE] backend/local: running validation operation") - validateDiags := tfCtx.Validate() + validateDiags := validateCtx.Validate() diags = diags.Append(validateDiags) } } From 99cd3ab223b8426ef1b264a7048ede81ab672d80 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 7 Aug 2020 14:04:40 -0400 Subject: [PATCH 3/3] fix command tests A number of tests had invalid configs or providers, but were never properly validated --- command/console_test.go | 24 +++++++++++++++++++++++- command/graph_test.go | 10 +++++----- command/testdata/apply-vars/main.tf | 2 +- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/command/console_test.go b/command/console_test.go index 5f1cfe240..b8e73fe33 100644 --- a/command/console_test.go +++ b/command/console_test.go @@ -8,8 +8,11 @@ import ( "strings" "testing" + "github.com/hashicorp/terraform/configs/configschema" "github.com/hashicorp/terraform/helper/copy" + "github.com/hashicorp/terraform/terraform" "github.com/mitchellh/cli" + "github.com/zclconf/go-cty/cty" ) // ConsoleCommand is tested primarily with tests in the "repl" package. @@ -60,6 +63,16 @@ func TestConsole_tfvars(t *testing.T) { } p := testProvider() + p.GetSchemaReturn = &terraform.ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_instance": { + Attributes: map[string]*configschema.Attribute{ + "value": {Type: cty.String, Optional: true}, + }, + }, + }, + } + ui := new(cli.MockUi) c := &ConsoleCommand{ Meta: Meta{ @@ -98,6 +111,15 @@ func TestConsole_unsetRequiredVars(t *testing.T) { defer testFixCwd(t, tmp, cwd) p := testProvider() + p.GetSchemaReturn = &terraform.ProviderSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_instance": { + Attributes: map[string]*configschema.Attribute{ + "value": {Type: cty.String, Optional: true}, + }, + }, + }, + } ui := new(cli.MockUi) c := &ConsoleCommand{ Meta: Meta{ @@ -142,7 +164,7 @@ func TestConsole_modules(t *testing.T) { defer os.RemoveAll(td) defer testChdir(t, td)() - p := testProvider() + p := applyFixtureProvider() ui := new(cli.MockUi) c := &ConsoleCommand{ diff --git a/command/graph_test.go b/command/graph_test.go index bb54d2d7a..c7696464f 100644 --- a/command/graph_test.go +++ b/command/graph_test.go @@ -20,7 +20,7 @@ func TestGraph(t *testing.T) { ui := new(cli.MockUi) c := &GraphCommand{ Meta: Meta{ - testingOverrides: metaOverridesForProvider(testProvider()), + testingOverrides: metaOverridesForProvider(applyFixtureProvider()), Ui: ui, }, } @@ -42,7 +42,7 @@ func TestGraph_multipleArgs(t *testing.T) { ui := new(cli.MockUi) c := &GraphCommand{ Meta: Meta{ - testingOverrides: metaOverridesForProvider(testProvider()), + testingOverrides: metaOverridesForProvider(applyFixtureProvider()), Ui: ui, }, } @@ -69,7 +69,7 @@ func TestGraph_noArgs(t *testing.T) { ui := new(cli.MockUi) c := &GraphCommand{ Meta: Meta{ - testingOverrides: metaOverridesForProvider(testProvider()), + testingOverrides: metaOverridesForProvider(applyFixtureProvider()), Ui: ui, }, } @@ -94,7 +94,7 @@ func TestGraph_noConfig(t *testing.T) { ui := new(cli.MockUi) c := &GraphCommand{ Meta: Meta{ - testingOverrides: metaOverridesForProvider(testProvider()), + testingOverrides: metaOverridesForProvider(applyFixtureProvider()), Ui: ui, }, } @@ -148,7 +148,7 @@ func TestGraph_plan(t *testing.T) { ui := new(cli.MockUi) c := &GraphCommand{ Meta: Meta{ - testingOverrides: metaOverridesForProvider(testProvider()), + testingOverrides: metaOverridesForProvider(applyFixtureProvider()), Ui: ui, }, } diff --git a/command/testdata/apply-vars/main.tf b/command/testdata/apply-vars/main.tf index 005abad09..1d6da85c3 100644 --- a/command/testdata/apply-vars/main.tf +++ b/command/testdata/apply-vars/main.tf @@ -1,5 +1,5 @@ variable "foo" {} resource "test_instance" "foo" { - value = "${var.foo}" + value = var.foo }