Merge pull request #25779 from hashicorp/jbardin/remove-state-attrs

Remove resource state attributes that are no longer in the schema

backend/local/backend_local.go

@@ -84,6 +84,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
@@ -101,9 +108,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() {
@@ -129,7 +145,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)
 		}
 	}

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{

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,
 		},
 	}

command/testdata/apply-vars/main.tf

@@ -1,5 +1,5 @@
 variable "foo" {}
 
 resource "test_instance" "foo" {
-    value = "${var.foo}"
+    value = var.foo
 }

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
+}

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")
+			}
+		})
+	}
+}