helper/resource: Ignore Removed attributes for ImportStateVerify

Due to the lossiness of our legacy models for diff and state, shimming a
diff and then creating a state from it produces a different result than
shimming a state directly. That means that ImportStateVerify no longer
works as expected if there are any Computed attributes in the schema where
d.Set isn't called during Read.

Fixing that for every case would require some risky changes to the shim
behavior, so we're instead going to ask provider developers to address it
by adding `d.Set` calls where needed, since that is the contract for
"Computed" anyway -- a default value should be produced during Create, and
thus by extension during Import.

However, since a common situation where this occurs is attributes marked
as "Removed", since all of the code that deals with them has generally
been deleted, we'll avoid problems in that case here by treating Removed
attributes as ignored for the purposes of ImportStateVerify.

This required exporting some functionality that was formerly unexported
in helper/schema, but it's a relatively harmless schema introspection
function so shouldn't be a big deal to export it.

builtin/providers/test/provider.go

@@ -20,6 +20,7 @@ func Provider() terraform.ResourceProvider {
 			"test_resource":                  testResource(),
 			"test_resource_gh12183":          testResourceGH12183(),
 			"test_resource_import_other":     testResourceImportOther(),
+			"test_resource_import_removed":   testResourceImportRemoved(),
 			"test_resource_with_custom_diff": testResourceCustomDiff(),
 			"test_resource_timeout":          testResourceTimeout(),
 			"test_resource_diff_suppress":    testResourceDiffSuppress(),

builtin/providers/test/resource_import_removed.go

@@ -0,0 +1,60 @@
+package test
+
+import (
+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func testResourceImportRemoved() *schema.Resource {
+	return &schema.Resource{
+		Create: testResourceImportRemovedCreate,
+		Read:   testResourceImportRemovedRead,
+		Delete: testResourceImportRemovedDelete,
+		Update: testResourceImportRemovedUpdate,
+
+		Importer: &schema.ResourceImporter{
+			State: testResourceImportRemovedImportState,
+		},
+
+		Schema: map[string]*schema.Schema{
+			"removed": {
+				Type:     schema.TypeInt,
+				Optional: true,
+				Computed: true,
+				Removed:  "do not use",
+			},
+		},
+	}
+}
+
+func testResourceImportRemovedImportState(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
+	var results []*schema.ResourceData
+
+	results = append(results, d)
+
+	{
+		other := testResourceDefaults()
+		od := other.Data(nil)
+		od.SetType("test_resource_import_removed")
+		od.SetId("foo")
+		results = append(results, od)
+	}
+
+	return results, nil
+}
+
+func testResourceImportRemovedCreate(d *schema.ResourceData, meta interface{}) error {
+	d.SetId("foo")
+	return testResourceImportRemovedRead(d, meta)
+}
+
+func testResourceImportRemovedUpdate(d *schema.ResourceData, meta interface{}) error {
+	return testResourceImportRemovedRead(d, meta)
+}
+
+func testResourceImportRemovedRead(d *schema.ResourceData, meta interface{}) error {
+	return nil
+}
+
+func testResourceImportRemovedDelete(d *schema.ResourceData, meta interface{}) error {
+	return nil
+}

builtin/providers/test/resource_import_removed_test.go

@@ -0,0 +1,41 @@
+package test
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestResourceImportRemoved(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_import_removed" "foo" {
+}
+				`),
+			},
+			{
+				ImportState:  true,
+				ResourceName: "test_resource_import_removed.foo",
+
+				// This is attempting to guard against regressions of:
+				// https://github.com/hashicorp/terraform/issues/20985
+				//
+				// Removed attributes are generally not populated during Create,
+				// Update, Read, or Import by provider code but due to our
+				// legacy diff format being lossy they end up getting populated
+				// with zero values during shimming in all cases except Import,
+				// which doesn't go through a diff.
+				//
+				// This is testing that the shimming inconsistency won't cause
+				// ImportStateVerify failures for these, since we now ignore
+				// attributes marked as Removed when comparing.
+				ImportStateVerify: true,
+			},
+		},
+	})
+}

helper/resource/testing_import_state.go

@@ -11,6 +11,7 @@ import (
 	"github.com/hashicorp/hcl2/hcl/hclsyntax"
 
 	"github.com/hashicorp/terraform/addrs"
+	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/hashicorp/terraform/states"
 	"github.com/hashicorp/terraform/terraform"
 )
@@ -130,6 +131,21 @@ func testStepImportState(
 					r.Primary.ID)
 			}
 
+			// We'll try our best to find the schema for this resource type
+			// so we can ignore Removed fields during validation. If we fail
+			// to find the schema then we won't ignore them and so the test
+			// will need to rely on explicit ImportStateVerifyIgnore, though
+			// this shouldn't happen in any reasonable case.
+			var rsrcSchema *schema.Resource
+			if providerAddr, diags := addrs.ParseAbsProviderConfigStr(r.Provider); !diags.HasErrors() {
+				providerType := providerAddr.ProviderConfig.Type
+				if provider, ok := step.providers[providerType]; ok {
+					if provider, ok := provider.(*schema.Provider); ok {
+						rsrcSchema = provider.ResourcesMap[r.Type]
+					}
+				}
+			}
+
 			// don't add empty flatmapped containers, so we can more easily
 			// compare the attributes
 			skipEmpty := func(k, v string) bool {
@@ -160,18 +176,39 @@ func testStepImportState(
 
 			// Remove fields we're ignoring
 			for _, v := range step.ImportStateVerifyIgnore {
-				for k, _ := range actual {
+				for k := range actual {
 					if strings.HasPrefix(k, v) {
 						delete(actual, k)
 					}
 				}
-				for k, _ := range expected {
+				for k := range expected {
 					if strings.HasPrefix(k, v) {
 						delete(expected, k)
 					}
 				}
 			}
 
+			// Also remove any attributes that are marked as "Removed" in the
+			// schema, if we have a schema to check that against.
+			if rsrcSchema != nil {
+				for k := range actual {
+					for _, schema := range rsrcSchema.SchemasForFlatmapPath(k) {
+						if schema.Removed != "" {
+							delete(actual, k)
+							break
+						}
+					}
+				}
+				for k := range expected {
+					for _, schema := range rsrcSchema.SchemasForFlatmapPath(k) {
+						if schema.Removed != "" {
+							delete(expected, k)
+							break
+						}
+					}
+				}
+			}
+
 			if !reflect.DeepEqual(actual, expected) {
 				// Determine only the different attributes
 				for k, v := range expected {

helper/schema/field_reader.go

@@ -3,6 +3,7 @@ package schema
 import (
 	"fmt"
 	"strconv"
+	"strings"
 )
 
 // FieldReaders are responsible for decoding fields out of data into
@@ -41,6 +42,13 @@ func (r *FieldReadResult) ValueOrZero(s *Schema) interface{} {
 	return s.ZeroValue()
 }
 
+// SchemasForFlatmapPath tries its best to find a sequence of schemas that
+// the given dot-delimited attribute path traverses through.
+func SchemasForFlatmapPath(path string, schemaMap map[string]*Schema) []*Schema {
+	parts := strings.Split(path, ".")
+	return addrToSchema(parts, schemaMap)
+}
+
 // addrToSchema finds the final element schema for the given address
 // and the given schema. It returns all the schemas that led to the final
 // schema. These are in order of the address (out to in).

helper/schema/resource.go

@@ -754,6 +754,13 @@ func (r *Resource) TestResourceData() *ResourceData {
 	}
 }
 
+// SchemasForFlatmapPath tries its best to find a sequence of schemas that
+// the given dot-delimited attribute path traverses through in the schema
+// of the receiving Resource.
+func (r *Resource) SchemasForFlatmapPath(path string) []*Schema {
+	return SchemasForFlatmapPath(path, r.Schema)
+}
+
 // Returns true if the resource is "top level" i.e. not a sub-resource.
 func (r *Resource) isTopLevel() bool {
 	// TODO: This is a heuristic; replace with a definitive attribute?