Merge pull request #24904 from hashicorp/jbardin/plan-data-sources

Evaluate data sources in plan when necessary

plans/changes.go

@@ -52,6 +52,22 @@ func (c *Changes) ResourceInstance(addr addrs.AbsResourceInstance) *ResourceInst
 	}
 
 	return nil
+
+}
+
+// InstancesForConfigResource returns the planned change for the current objects
+// of the resource instances of the given address, if any. Returns nil if no
+// changes are planned.
+func (c *Changes) InstancesForConfigResource(addr addrs.ConfigResource) []*ResourceInstanceChangeSrc {
+	var changes []*ResourceInstanceChangeSrc
+	for _, rc := range c.Resources {
+		resAddr := rc.Addr.ContainingResource().Config()
+		if resAddr.Equal(addr) && rc.DeposedKey == states.NotDeposed {
+			changes = append(changes, rc)
+		}
+	}
+
+	return changes
 }
 
 // ResourceInstanceDeposed returns the plan change of a deposed object of

plans/changes_sync.go

@@ -62,6 +62,29 @@ func (cs *ChangesSync) GetResourceInstanceChange(addr addrs.AbsResourceInstance,
 	panic(fmt.Sprintf("unsupported generation value %#v", gen))
 }
 
+// GetChangesForConfigResource searched the set of resource instance
+// changes and returns all changes related to a given configuration address.
+// This is be used to find possible changes related to a configuration
+// reference.
+//
+// If no such changes exist, nil is returned.
+//
+// The returned objects are a deep copy of the change recorded in the plan, so
+// callers may mutate them although it's generally better (less confusing) to
+// treat planned changes as immutable after they've been initially constructed.
+func (cs *ChangesSync) GetChangesForConfigResource(addr addrs.ConfigResource) []*ResourceInstanceChangeSrc {
+	if cs == nil {
+		panic("GetChangesForConfigResource on nil ChangesSync")
+	}
+	cs.lock.Lock()
+	defer cs.lock.Unlock()
+	var changes []*ResourceInstanceChangeSrc
+	for _, c := range cs.changes.InstancesForConfigResource(addr) {
+		changes = append(changes, c.DeepCopy())
+	}
+	return changes
+}
+
 // RemoveResourceInstanceChange searches the set of resource instance changes
 // for one matching the given address and generation, and removes it from the
 // set if it exists.

terraform/context_apply_test.go

@@ -8724,7 +8724,20 @@ func TestContext2Apply_destroyNestedModuleWithAttrsReferencingResource(t *testin
 // that resource to be applied first.
 func TestContext2Apply_dataDependsOn(t *testing.T) {
 	p := testProvider("null")
-	m := testModule(t, "apply-data-depends-on")
+	m := testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "null_instance" "write" {
+  foo = "attribute"
+}
+
+data "null_data_source" "read" {
+  depends_on = ["null_instance.write"]
+}
+
+resource "null_instance" "depends" {
+  foo = data.null_data_source.read.foo
+}
+`})
 
 	ctx := testContext2(t, &ContextOpts{
 		Config: m,
@@ -8782,6 +8795,63 @@ func TestContext2Apply_dataDependsOn(t *testing.T) {
 	if actual != expected {
 		t.Fatalf("bad:\n%s", strings.TrimSpace(state.String()))
 	}
+
+	// run another plan to make sure the data source doesn't show as a change
+	plan, diags := ctx.Plan()
+	assertNoErrors(t, diags)
+
+	for _, c := range plan.Changes.Resources {
+		if c.Action != plans.NoOp {
+			t.Fatalf("unexpected change for %s", c.Addr)
+		}
+	}
+
+	// now we cause a change in the first resource, which should trigger a plan
+	// in the data source, and the resource that depends on the data source
+	// must plan a change as well.
+	m = testModuleInline(t, map[string]string{
+		"main.tf": `
+resource "null_instance" "write" {
+  foo = "new"
+}
+
+data "null_data_source" "read" {
+  depends_on = ["null_instance.write"]
+}
+
+resource "null_instance" "depends" {
+  foo = data.null_data_source.read.foo
+}
+`})
+
+	p.ApplyFn = func(info *InstanceInfo, s *InstanceState, d *InstanceDiff) (*InstanceState, error) {
+		// the side effect of the resource being applied
+		provisionerOutput = "APPLIED_AGAIN"
+		return testApplyFn(info, s, d)
+	}
+
+	ctx = testContext2(t, &ContextOpts{
+		Config: m,
+		State:  state,
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("null"): testProviderFuncFixed(p),
+		},
+	})
+
+	plan, diags = ctx.Plan()
+	assertNoErrors(t, diags)
+
+	expectedChanges := map[string]plans.Action{
+		"null_instance.write":        plans.Update,
+		"data.null_data_source.read": plans.Read,
+		"null_instance.depends":      plans.Update,
+	}
+
+	for _, c := range plan.Changes.Resources {
+		if c.Action != expectedChanges[c.Addr.String()] {
+			t.Errorf("unexpected %s for %s", c.Action, c.Addr)
+		}
+	}
 }
 
 func TestContext2Apply_terraformWorkspace(t *testing.T) {

terraform/context_plan_test.go

@@ -1893,9 +1893,9 @@ func TestContext2Plan_computedInFunction(t *testing.T) {
 	assertNoErrors(t, diags)
 
 	if p.ReadDataSourceCalled {
-		t.Fatalf("ReadDataSource was called on provider during plan; should not have been called")
+		// there was no config change to read during plan
+		t.Fatalf("ReadDataSource should not have been called")
 	}
-
 }
 
 func TestContext2Plan_computedDataCountResource(t *testing.T) {
@@ -1993,6 +1993,7 @@ func TestContext2Plan_dataResourceBecomesComputed(t *testing.T) {
 		DataSources: map[string]*configschema.Block{
 			"aws_data_source": {
 				Attributes: map[string]*configschema.Attribute{
+					"id":  {Type: cty.String, Computed: true},
 					"foo": {Type: cty.String, Optional: true},
 				},
 			},
@@ -4992,8 +4993,10 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 		}
 	}
 	p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse {
+		cfg := req.Config.AsValueMap()
+		cfg["id"] = cty.StringVal("data_id")
 		return providers.ReadDataSourceResponse{
-			Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("ReadDataSource called, but should not have been")),
+			State: cty.ObjectVal(cfg),
 		}
 	}
 
@@ -5010,9 +5013,6 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 	// thus the plan call below is forced to produce a deferred read action.
 
 	plan, diags := ctx.Plan()
-	if p.ReadDataSourceCalled {
-		t.Errorf("ReadDataSource was called on the provider, but should not have been because we didn't refresh")
-	}
 	if diags.HasErrors() {
 		t.Fatalf("unexpected errors: %s", diags.Err())
 	}
@@ -5042,7 +5042,7 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 				}
 				checkVals(t, objectVal(t, schema, map[string]cty.Value{
 					"num":      cty.StringVal("2"),
-					"computed": cty.UnknownVal(cty.String),
+					"computed": cty.StringVal("data_id"),
 				}), ric.After)
 			case "aws_instance.foo[1]":
 				if res.Action != plans.Create {
@@ -5050,18 +5050,14 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 				}
 				checkVals(t, objectVal(t, schema, map[string]cty.Value{
 					"num":      cty.StringVal("2"),
-					"computed": cty.UnknownVal(cty.String),
+					"computed": cty.StringVal("data_id"),
 				}), ric.After)
 			case "data.aws_vpc.bar[0]":
 				if res.Action != plans.Read {
 					t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action)
 				}
 				checkVals(t, objectVal(t, schema, map[string]cty.Value{
-					// In a normal flow we would've read an exact value in
+					"id":  cty.StringVal("data_id"),
-					// ReadDataSource, but because this test doesn't run
-					// cty.Refresh we have no opportunity to do that lookup
-					// and a deferred read is forced.
-					"id":  cty.UnknownVal(cty.String),
 					"foo": cty.StringVal("0"),
 				}), ric.After)
 			case "data.aws_vpc.bar[1]":
@@ -5069,11 +5065,7 @@ func TestContext2Plan_createBeforeDestroy_depends_datasource(t *testing.T) {
 					t.Fatalf("resource %s should be read, got %s", ric.Addr, ric.Action)
 				}
 				checkVals(t, objectVal(t, schema, map[string]cty.Value{
-					// In a normal flow we would've read an exact value in
+					"id":  cty.StringVal("data_id"),
-					// ReadDataSource, but because this test doesn't run
-					// cty.Refresh we have no opportunity to do that lookup
-					// and a deferred read is forced.
-					"id":  cty.UnknownVal(cty.String),
 					"foo": cty.StringVal("1"),
 				}), ric.After)
 			default:
@@ -5513,11 +5505,18 @@ func TestContext2Plan_invalidOutput(t *testing.T) {
 data "aws_data_source" "name" {}
 
 output "out" {
-  value = "${data.aws_data_source.name.missing}"
+  value = data.aws_data_source.name.missing
 }`,
 	})
 
 	p := testProvider("aws")
+	p.ReadDataSourceResponse = providers.ReadDataSourceResponse{
+		State: cty.ObjectVal(map[string]cty.Value{
+			"id":  cty.StringVal("data_id"),
+			"foo": cty.StringVal("foo"),
+		}),
+	}
+
 	ctx := testContext2(t, &ContextOpts{
 		Config: m,
 		Providers: map[addrs.Provider]providers.Factory{
@@ -5558,6 +5557,13 @@ resource "aws_instance" "foo" {
 	})
 
 	p := testProvider("aws")
+	p.ReadDataSourceResponse = providers.ReadDataSourceResponse{
+		State: cty.ObjectVal(map[string]cty.Value{
+			"id":  cty.StringVal("data_id"),
+			"foo": cty.StringVal("foo"),
+		}),
+	}
+
 	ctx := testContext2(t, &ContextOpts{
 		Config: m,
 		Providers: map[addrs.Provider]providers.Factory{

terraform/context_refresh_test.go

@@ -949,32 +949,11 @@ func TestContext2Refresh_dataState(t *testing.T) {
 
 	p.ReadDataSourceFn = func(req providers.ReadDataSourceRequest) providers.ReadDataSourceResponse {
 		m := req.Config.AsValueMap()
-		m["inputs"] = cty.MapVal(map[string]cty.Value{"test": cty.StringVal("yes")})
 		readStateVal = cty.ObjectVal(m)
 
 		return providers.ReadDataSourceResponse{
 			State: readStateVal,
 		}
-
-		// FIXME: should the "outputs" value here be added to the reutnred state?
-		// Attributes: map[string]*ResourceAttrDiff{
-		// 	"inputs.#": {
-		// 		Old:  "0",
-		// 		New:  "1",
-		// 		Type: DiffAttrInput,
-		// 	},
-		// 	"inputs.test": {
-		// 		Old:  "",
-		// 		New:  "yes",
-		// 		Type: DiffAttrInput,
-		// 	},
-		// 	"outputs.#": {
-		// 		Old:         "",
-		// 		New:         "",
-		// 		NewComputed: true,
-		// 		Type:        DiffAttrOutput,
-		// 	},
-		// },
 	}
 
 	s, diags := ctx.Refresh()
@@ -986,14 +965,6 @@ func TestContext2Refresh_dataState(t *testing.T) {
 		t.Fatal("ReadDataSource should have been called")
 	}
 
-	// mod := s.RootModule()
-	// if got := mod.Resources["data.null_data_source.testing"].Primary.ID; got != "-" {
-	// 	t.Fatalf("resource id is %q; want %s", got, "-")
-	// }
-	// if !reflect.DeepEqual(mod.Resources["data.null_data_source.testing"].Primary, p.ReadDataApplyReturn) {
-	// 	t.Fatalf("bad: %#v", mod.Resources)
-	// }
-
 	mod := s.RootModule()
 
 	newState, err := mod.Resources["data.null_data_source.testing"].Instances[addrs.NoKey].Current.Decode(schema.ImpliedType())
@@ -1612,6 +1583,11 @@ func TestContext2Refresh_dataResourceDependsOn(t *testing.T) {
 		},
 	}
 	p.DiffFn = testDiffFn
+	p.ReadDataSourceResponse = providers.ReadDataSourceResponse{
+		State: cty.ObjectVal(map[string]cty.Value{
+			"compute": cty.StringVal("value"),
+		}),
+	}
 
 	state := states.NewState()
 	root := state.EnsureModule(addrs.RootModuleInstance)

terraform/context_test.go

@@ -693,11 +693,12 @@ func testProviderSchema(name string) *ProviderSchema {
 				Attributes: map[string]*configschema.Attribute{
 					"id": {
 						Type:     cty.String,
-						Optional: true,
+						Computed: true,
 					},
 					"foo": {
 						Type:     cty.String,
 						Optional: true,
+						Computed: true,
 					},
 				},
 			},

terraform/eval_read_data.go

@@ -16,10 +16,9 @@ import (
 	"github.com/hashicorp/terraform/tfdiags"
 )
 
-// EvalReadData is an EvalNode implementation that deals with the main part
+// evalReadData implements shared methods and data for the individual  data
-// of the data resource lifecycle: either actually reading from the data source
+// source eval nodes.
-// or generating a plan to do so.
+type evalReadData struct {
-type EvalReadData struct {
 	Addr           addrs.ResourceInstance
 	Config         *configs.Resource
 	Provider       *providers.Interface
@@ -34,175 +33,59 @@ type EvalReadData struct {
 	// in this planned change.
 	Planned **plans.ResourceInstanceChange
 
-	// ForcePlanRead, if true, overrides the usual behavior of immediately
+	// State is the current state for the data source, and is updated once the
-	// reading from the data source where possible, instead forcing us to
+	// new state has been read.
-	// _always_ generate a plan. This is used during the plan walk, since we
+	// While data sources are read-only, we need to start with the prior state
-	// mustn't actually apply anything there. (The resulting state doesn't
+	// to determine if we have a change or not.  If we needed to read a new
-	// get persisted)
+	// value, but it still matches the previous state, then we can record a
-	ForcePlanRead bool
+	// NoNop change. If the states don't match then we record a Read change so
+	// that the new value is applied to the state.
+	State **states.ResourceInstanceObject
 
-	// The result from this EvalNode has a few different possibilities
+	// Output change records any change for this data source, which is
-	// depending on the input:
+	// interpreted differently than changes for managed resources.
-	// - If Planned is nil then we assume we're aiming to _produce_ the plan,
+	// - During Refresh, this change is only used to correctly evaluate
-	//   and so the following two outcomes are possible:
+	// references to the data source, but it is not saved.
-	//     - OutputChange.Action is plans.NoOp and OutputState is the complete
+	// - If a planned change has the action of plans.Read, it indicates that the
-	//       result of reading from the data source. This is the easy path.
+	// data source could not be evaluated yet, and reading is being deferred to
-	//     - OutputChange.Action is plans.Read and OutputState is a planned
+	// apply.
-	//       object placeholder (states.ObjectPlanned). In this case, the
+	// - If planned action is plans.Update, it indicates that the data source
-	//       returned change must be recorded in the overral changeset and
+	// was read, and the result needs to be stored in state during apply.
-	//       eventually passed to another instance of this struct during the
-	//       apply walk.
-	// - If Planned is non-nil then we assume we're aiming to complete a
-	//   planned read from an earlier plan walk. In this case the only possible
-	//   non-error outcome is to set Output.Action (if non-nil) to a plans.NoOp
-	//   change and put the complete resulting state in OutputState, ready to
-	//   be saved in the overall state and used for expression evaluation.
 	OutputChange **plans.ResourceInstanceChange
-	OutputValue       *cty.Value
-	OutputConfigValue *cty.Value
-	OutputState       **states.ResourceInstanceObject
-}
-
-func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
-	absAddr := n.Addr.Absolute(ctx.Path())
-	log.Printf("[TRACE] EvalReadData: working on %s", absAddr)
-
-	if n.ProviderSchema == nil || *n.ProviderSchema == nil {
-		return nil, fmt.Errorf("provider schema not available for %s", n.Addr)
 }
 
+// readDataSource handles everything needed to call ReadDataSource on the provider.
+// A previously evaluated configVal can be passed in, or a new one is generated
+// from the resource configuration.
+func (n *evalReadData) readDataSource(ctx EvalContext, configVal cty.Value) (cty.Value, tfdiags.Diagnostics) {
 	var diags tfdiags.Diagnostics
-	var change *plans.ResourceInstanceChange
+	var newVal cty.Value
-	var configVal cty.Value
-
-	// TODO: Do we need to handle Delete changes here? EvalReadDataDiff and
-	// EvalReadDataApply did, but it seems like we should handle that via a
-	// separate mechanism since it boils down to just deleting the object from
-	// the state... and we do that on every plan anyway, forcing the data
-	// resource to re-read.
 
 	config := *n.Config
+	absAddr := n.Addr.Absolute(ctx.Path())
+
+	if n.ProviderSchema == nil || *n.ProviderSchema == nil {
+		diags = diags.Append(fmt.Errorf("provider schema not available for %s", n.Addr))
+		return newVal, diags
+	}
+
 	provider := *n.Provider
+
 	providerSchema := *n.ProviderSchema
 	schema, _ := providerSchema.SchemaForResourceAddr(n.Addr.ContainingResource())
 	if schema == nil {
 		// Should be caught during validation, so we don't bother with a pretty error here
-		return nil, fmt.Errorf("provider %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type)
+		diags = diags.Append(fmt.Errorf("provider %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type))
+		return newVal, diags
 	}
 
-	// We'll always start by evaluating the configuration. What we do after
+	metaConfigVal, metaDiags := n.providerMetas(ctx)
-	// that will depend on the evaluation result along with what other inputs
+	diags = diags.Append(metaDiags)
-	// we were given.
+	if diags.HasErrors() {
-	objTy := schema.ImpliedType()
+		return newVal, diags
-	priorVal := cty.NullVal(objTy) // for data resources, prior is always null because we start fresh every time
-
-	forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx)
-	keyData := EvalDataForInstanceKey(n.Addr.Key, forEach)
-
-	var configDiags tfdiags.Diagnostics
-	configVal, _, configDiags = ctx.EvaluateBlock(config.Config, schema, nil, keyData)
-	diags = diags.Append(configDiags)
-	if configDiags.HasErrors() {
-		return nil, diags.Err()
 	}
 
-	metaConfigVal := cty.NullVal(cty.DynamicPseudoType)
+	log.Printf("[TRACE] EvalReadData: Re-validating config for %s", absAddr)
-	if n.ProviderMetas != nil {
-		if m, ok := n.ProviderMetas[n.ProviderAddr.Provider]; ok && m != nil {
-			// if the provider doesn't support this feature, throw an error
-			if (*n.ProviderSchema).ProviderMeta == nil {
-				diags = diags.Append(&hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  fmt.Sprintf("Provider %s doesn't support provider_meta", n.ProviderAddr.Provider.String()),
-					Detail:   fmt.Sprintf("The resource %s belongs to a provider that doesn't support provider_meta blocks", n.Addr),
-					Subject:  &m.ProviderRange,
-				})
-			} else {
-				var configDiags tfdiags.Diagnostics
-				metaConfigVal, _, configDiags = ctx.EvaluateBlock(m.Config, (*n.ProviderSchema).ProviderMeta, nil, EvalDataForNoInstanceKey)
-				diags = diags.Append(configDiags)
-				if configDiags.HasErrors() {
-					return nil, diags.Err()
-				}
-			}
-		}
-	}
-
-	proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal)
-
-	// If our configuration contains any unknown values then we must defer the
-	// read to the apply phase by producing a "Read" change for this resource,
-	// and a placeholder value for it in the state.
-	if n.ForcePlanRead || !configVal.IsWhollyKnown() {
-		// If the configuration is still unknown when we're applying a planned
-		// change then that indicates a bug in Terraform, since we should have
-		// everything resolved by now.
-		if n.Planned != nil && *n.Planned != nil {
-			return nil, fmt.Errorf(
-				"configuration for %s still contains unknown values during apply (this is a bug in Terraform; please report it!)",
-				absAddr,
-			)
-		}
-		if n.ForcePlanRead {
-			log.Printf("[TRACE] EvalReadData: %s configuration is fully known, but we're forcing a read plan to be created", absAddr)
-		} else {
-			log.Printf("[TRACE] EvalReadData: %s configuration not fully known yet, so deferring to apply phase", absAddr)
-		}
-
-		err := ctx.Hook(func(h Hook) (HookAction, error) {
-			return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal)
-		})
-		if err != nil {
-			return nil, err
-		}
-
-		change = &plans.ResourceInstanceChange{
-			Addr:         absAddr,
-			ProviderAddr: n.ProviderAddr,
-			Change: plans.Change{
-				Action: plans.Read,
-				Before: priorVal,
-				After:  proposedNewVal,
-			},
-		}
-
-		err = ctx.Hook(func(h Hook) (HookAction, error) {
-			return h.PostDiff(absAddr, states.CurrentGen, change.Action, priorVal, proposedNewVal)
-		})
-		if err != nil {
-			return nil, err
-		}
-
-		if n.OutputChange != nil {
-			*n.OutputChange = change
-		}
-		if n.OutputValue != nil {
-			*n.OutputValue = change.After
-		}
-		if n.OutputConfigValue != nil {
-			*n.OutputConfigValue = configVal
-		}
-		if n.OutputState != nil {
-			state := &states.ResourceInstanceObject{
-				Value:  change.After,
-				Status: states.ObjectPlanned, // because the partial value in the plan must be used for now
-			}
-			*n.OutputState = state
-		}
-
-		return nil, diags.ErrWithWarnings()
-	}
-
-	if n.Planned != nil && *n.Planned != nil && (*n.Planned).Action != plans.Read {
-		// If any other action gets in here then that's always a bug; this
-		// EvalNode only deals with reading.
-		return nil, fmt.Errorf(
-			"invalid action %s for %s: only Read is supported (this is a bug in Terraform; please report it!)",
-			(*n.Planned).Action, absAddr,
-		)
-	}
-
-	log.Printf("[TRACE] Re-validating config for %s", absAddr)
 	validateResp := provider.ValidateDataSourceConfig(
 		providers.ValidateDataSourceConfigRequest{
 			TypeName: n.Addr.Resource.Type,
@@ -210,32 +93,23 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 		},
 	)
 	if validateResp.Diagnostics.HasErrors() {
-		return nil, validateResp.Diagnostics.InConfigBody(n.Config.Config).Err()
+		return newVal, validateResp.Diagnostics.InConfigBody(config.Config)
 	}
 
 	// If we get down here then our configuration is complete and we're read
 	// to actually call the provider to read the data.
 	log.Printf("[TRACE] EvalReadData: %s configuration is complete, so reading from provider", absAddr)
 
-	err := ctx.Hook(func(h Hook) (HookAction, error) {
-		// We don't have a state yet, so we'll just give the hook an
-		// empty one to work with.
-		return h.PreRefresh(absAddr, states.CurrentGen, cty.NullVal(cty.DynamicPseudoType))
-	})
-	if err != nil {
-		return nil, err
-	}
-
 	resp := provider.ReadDataSource(providers.ReadDataSourceRequest{
 		TypeName:     n.Addr.Resource.Type,
 		Config:       configVal,
 		ProviderMeta: metaConfigVal,
 	})
-	diags = diags.Append(resp.Diagnostics.InConfigBody(n.Config.Config))
+	diags = diags.Append(resp.Diagnostics.InConfigBody(config.Config))
 	if diags.HasErrors() {
-		return nil, diags.Err()
+		return newVal, diags
 	}
-	newVal := resp.State
+	newVal = resp.State
 	if newVal == cty.NilVal {
 		// This can happen with incompletely-configured mocks. We'll allow it
 		// and treat it as an alias for a properly-typed null value.
@@ -253,7 +127,7 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 		))
 	}
 	if diags.HasErrors() {
-		return nil, diags.Err()
+		return newVal, diags
 	}
 
 	if newVal.IsNull() {
@@ -266,7 +140,8 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 			),
 		))
 	}
-	if !newVal.IsWhollyKnown() {
+
+	if !newVal.IsNull() && !newVal.IsWhollyKnown() {
 		diags = diags.Append(tfdiags.Sourceless(
 			tfdiags.Error,
 			"Provider produced invalid object",
@@ -285,152 +160,133 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 		newVal = cty.UnknownAsNull(newVal)
 	}
 
-	// Since we've completed the read, we actually have no change to make, but
+	return newVal, diags
-	// we'll produce a NoOp one anyway to preserve the usual flow of the
-	// plan phase and allow it to produce a complete plan.
-	change = &plans.ResourceInstanceChange{
-		Addr:         absAddr,
-		ProviderAddr: n.ProviderAddr,
-		Change: plans.Change{
-			Action: plans.NoOp,
-			Before: newVal,
-			After:  newVal,
-		},
-	}
-	state := &states.ResourceInstanceObject{
-		Value:  change.After,
-		Status: states.ObjectReady, // because we completed the read from the provider
 }
 
-	err = ctx.Hook(func(h Hook) (HookAction, error) {
+func (n *evalReadData) providerMetas(ctx EvalContext) (cty.Value, tfdiags.Diagnostics) {
-		return h.PostRefresh(absAddr, states.CurrentGen, change.Before, newVal)
-	})
-	if err != nil {
-		return nil, err
-	}
-
-	if n.OutputChange != nil {
-		*n.OutputChange = change
-	}
-	if n.OutputValue != nil {
-		*n.OutputValue = change.After
-	}
-	if n.OutputConfigValue != nil {
-		*n.OutputConfigValue = configVal
-	}
-	if n.OutputState != nil {
-		*n.OutputState = state
-	}
-
-	return nil, diags.ErrWithWarnings()
-}
-
-// EvalReadDataApply is an EvalNode implementation that executes a data
-// resource's ReadDataApply method to read data from the data source.
-type EvalReadDataApply struct {
-	Addr           addrs.ResourceInstance
-	Provider       *providers.Interface
-	ProviderAddr   addrs.AbsProviderConfig
-	ProviderMeta   *configs.ProviderMeta
-	ProviderSchema **ProviderSchema
-	Output         **states.ResourceInstanceObject
-	Config         *configs.Resource
-	Change         **plans.ResourceInstanceChange
-}
-
-func (n *EvalReadDataApply) Eval(ctx EvalContext) (interface{}, error) {
-	provider := *n.Provider
-	change := *n.Change
-	providerSchema := *n.ProviderSchema
-	absAddr := n.Addr.Absolute(ctx.Path())
-
 	var diags tfdiags.Diagnostics
-
-	// If the diff is for *destroying* this resource then we'll
-	// just drop its state and move on, since data resources don't
-	// support an actual "destroy" action.
-	if change != nil && change.Action == plans.Delete {
-		if n.Output != nil {
-			*n.Output = nil
-		}
-		return nil, nil
-	}
-
 	metaConfigVal := cty.NullVal(cty.DynamicPseudoType)
-	if n.ProviderMeta != nil {
+	if n.ProviderMetas != nil {
+		if m, ok := n.ProviderMetas[n.ProviderAddr.Provider]; ok && m != nil {
 			// if the provider doesn't support this feature, throw an error
 			if (*n.ProviderSchema).ProviderMeta == nil {
 				diags = diags.Append(&hcl.Diagnostic{
 					Severity: hcl.DiagError,
 					Summary:  fmt.Sprintf("Provider %s doesn't support provider_meta", n.ProviderAddr.Provider.String()),
 					Detail:   fmt.Sprintf("The resource %s belongs to a provider that doesn't support provider_meta blocks", n.Addr),
-				Subject:  &n.ProviderMeta.ProviderRange,
+					Subject:  &m.ProviderRange,
 				})
 			} else {
 				var configDiags tfdiags.Diagnostics
-			metaConfigVal, _, configDiags = ctx.EvaluateBlock(n.ProviderMeta.Config, (*n.ProviderSchema).ProviderMeta, nil, EvalDataForNoInstanceKey)
+				metaConfigVal, _, configDiags = ctx.EvaluateBlock(m.Config, (*n.ProviderSchema).ProviderMeta, nil, EvalDataForNoInstanceKey)
 				diags = diags.Append(configDiags)
-			if configDiags.HasErrors() {
-				return nil, diags.Err()
 			}
 		}
 	}
+	return metaConfigVal, diags
+}
 
-	// For the purpose of external hooks we present a data apply as a
+// evalReadDataRefresh is an EvalNode implementation that handled the data
-	// "Refresh" rather than an "Apply" because creating a data source
+// resource lifecycle during refresh
-	// is presented to users/callers as a "read" operation.
+type evalReadDataRefresh struct {
-	err := ctx.Hook(func(h Hook) (HookAction, error) {
+	evalReadData
-		// We don't have a state yet, so we'll just give the hook an
-		// empty one to work with.
-		return h.PreRefresh(absAddr, states.CurrentGen, cty.NullVal(cty.DynamicPseudoType))
-	})
-	if err != nil {
-		return nil, err
 }
 
-	resp := provider.ReadDataSource(providers.ReadDataSourceRequest{
+func (n *evalReadDataRefresh) Eval(ctx EvalContext) (interface{}, error) {
-		TypeName:     n.Addr.Resource.Type,
+	var diags tfdiags.Diagnostics
-		Config:       change.After,
+
-		ProviderMeta: metaConfigVal,
+	if n.ProviderSchema == nil || *n.ProviderSchema == nil {
-	})
+		return nil, fmt.Errorf("provider schema not available for %s", n.Addr)
-	diags = diags.Append(resp.Diagnostics.InConfigBody(n.Config.Config))
-	if diags.HasErrors() {
-		return nil, diags.Err()
 	}
 
+	absAddr := n.Addr.Absolute(ctx.Path())
+	config := *n.Config
+	providerSchema := *n.ProviderSchema
 	schema, _ := providerSchema.SchemaForResourceAddr(n.Addr.ContainingResource())
 	if schema == nil {
 		// Should be caught during validation, so we don't bother with a pretty error here
-		return nil, fmt.Errorf("provider does not support data source %q", n.Addr.Resource.Type)
+		return nil, fmt.Errorf("provider %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type)
 	}
 
-	newVal := resp.State
+	objTy := schema.ImpliedType()
-	for _, err := range newVal.Type().TestConformance(schema.ImpliedType()) {
+	priorVal := cty.NullVal(objTy)
-		diags = diags.Append(tfdiags.Sourceless(
+	if n.State != nil && *n.State != nil {
-			tfdiags.Error,
+		priorVal = (*n.State).Value
-			"Provider produced invalid object",
-			fmt.Sprintf(
-				"Provider %q planned an invalid value for %s. The result could not be saved.\n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.",
-				n.ProviderAddr.Provider.String(), tfdiags.FormatErrorPrefixed(err, absAddr.String()),
-			),
-		))
-	}
-	if diags.HasErrors() {
-		return nil, diags.Err()
 	}
 
-	err = ctx.Hook(func(h Hook) (HookAction, error) {
+	forEach, _ := evaluateForEachExpression(config.ForEach, ctx)
-		return h.PostRefresh(absAddr, states.CurrentGen, change.Before, newVal)
+	keyData := EvalDataForInstanceKey(n.Addr.Key, forEach)
-	})
+
-	if err != nil {
+	configVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData)
-		return nil, err
+	diags = diags.Append(configDiags)
+	if configDiags.HasErrors() {
+		return nil, diags.ErrWithWarnings()
 	}
 
-	if n.Output != nil {
+	configKnown := configVal.IsWhollyKnown()
-		*n.Output = &states.ResourceInstanceObject{
+	// If our configuration contains any unknown values, then we must defer the
-			Value:  newVal,
+	// read until plan or apply. If we've never read this data source and we
-			Status: states.ObjectReady,
+	// have any depends_on, we will have to defer reading until plan to resolve
+	// the dependency changes.
+	// Assuming we can read the data source with depends_on if we have
+	// existing state is a compromise to prevent data sources from continually
+	// showing a diff. We have to make the assumption that if we have a prior
+	// state, since there are no prior dependency changes happening during
+	// refresh, that we can read this resource. If there are dependency updates
+	// in the config, they we be discovered in plan and the data source will be
+	// read again.
+	if !configKnown || (priorVal.IsNull() && len(n.Config.DependsOn) > 0) {
+		if configKnown {
+			log.Printf("[TRACE] evalReadDataRefresh: %s configuration is fully known, but we're forcing a read plan to be created", absAddr)
+		} else {
+			log.Printf("[TRACE] evalReadDataRefresh: %s configuration not fully known yet, so deferring to apply phase", absAddr)
 		}
+
+		// We need to store a change so tat other references to this data
+		// source can resolve correctly, since the state is not going to be up
+		// to date.
+		*n.OutputChange = &plans.ResourceInstanceChange{
+			Addr:         absAddr,
+			ProviderAddr: n.ProviderAddr,
+			Change: plans.Change{
+				Action: plans.Read,
+				Before: priorVal,
+				After:  objchange.PlannedDataResourceObject(schema, configVal),
+			},
+		}
+
+		*n.State = &states.ResourceInstanceObject{
+			Value:  cty.NullVal(objTy),
+			Status: states.ObjectPlanned,
+		}
+
+		return nil, diags.ErrWithWarnings()
+	}
+
+	if err := ctx.Hook(func(h Hook) (HookAction, error) {
+		return h.PreRefresh(absAddr, states.CurrentGen, priorVal)
+	}); err != nil {
+		diags = diags.Append(err)
+		return nil, diags.ErrWithWarnings()
+	}
+
+	newVal, readDiags := n.readDataSource(ctx, configVal)
+	diags = diags.Append(readDiags)
+	if diags.HasErrors() {
+		return nil, diags.ErrWithWarnings()
+	}
+
+	// This may still have been refreshed with references to resources that
+	// will be updated, but that will be caught as a change during plan.
+	*n.State = &states.ResourceInstanceObject{
+		Value:  newVal,
+		Status: states.ObjectReady,
+	}
+
+	if err := ctx.Hook(func(h Hook) (HookAction, error) {
+		return h.PostRefresh(absAddr, states.CurrentGen, priorVal, newVal)
+	}); err != nil {
+		diags = diags.Append(err)
 	}
 
 	return nil, diags.ErrWithWarnings()

terraform/eval_read_data_apply.go

@@ -0,0 +1,98 @@
+package terraform
+
+import (
+	"fmt"
+
+	"github.com/hashicorp/terraform/plans"
+	"github.com/hashicorp/terraform/states"
+	"github.com/hashicorp/terraform/tfdiags"
+)
+
+// evalReadDataApply is an EvalNode implementation that deals with the main part
+// of the data resource lifecycle: either actually reading from the data source
+// or generating a plan to do so.
+type evalReadDataApply struct {
+	evalReadData
+}
+
+func (n *evalReadDataApply) Eval(ctx EvalContext) (interface{}, error) {
+	absAddr := n.Addr.Absolute(ctx.Path())
+
+	var diags tfdiags.Diagnostics
+
+	var planned *plans.ResourceInstanceChange
+	if n.Planned != nil {
+		planned = *n.Planned
+	}
+
+	if n.ProviderSchema == nil || *n.ProviderSchema == nil {
+		return nil, fmt.Errorf("provider schema not available for %s", n.Addr)
+	}
+
+	if planned != nil && planned.Action != plans.Read {
+		// If any other action gets in here then that's always a bug; this
+		// EvalNode only deals with reading.
+		return nil, fmt.Errorf(
+			"invalid action %s for %s: only Read is supported (this is a bug in Terraform; please report it!)",
+			planned.Action, absAddr,
+		)
+	}
+
+	if err := ctx.Hook(func(h Hook) (HookAction, error) {
+		return h.PreApply(absAddr, states.CurrentGen, planned.Action, planned.Before, planned.After)
+	}); err != nil {
+		return nil, err
+	}
+
+	// We have a change and it is complete, which means we read the data
+	// source during plan and only need to store it in state.
+	if planned.After.IsWhollyKnown() {
+		if err := ctx.Hook(func(h Hook) (HookAction, error) {
+			return h.PostApply(absAddr, states.CurrentGen, planned.After, nil)
+		}); err != nil {
+			diags = diags.Append(err)
+		}
+
+		*n.State = &states.ResourceInstanceObject{
+			Value:  planned.After,
+			Status: states.ObjectReady,
+		}
+		return nil, diags.ErrWithWarnings()
+	}
+
+	config := *n.Config
+	providerSchema := *n.ProviderSchema
+	schema, _ := providerSchema.SchemaForResourceAddr(n.Addr.ContainingResource())
+	if schema == nil {
+		// Should be caught during validation, so we don't bother with a pretty error here
+		return nil, fmt.Errorf("provider %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type)
+	}
+
+	forEach, _ := evaluateForEachExpression(config.ForEach, ctx)
+	keyData := EvalDataForInstanceKey(n.Addr.Key, forEach)
+
+	configVal, _, configDiags := ctx.EvaluateBlock(config.Config, schema, nil, keyData)
+	diags = diags.Append(configDiags)
+	if configDiags.HasErrors() {
+		return nil, diags.ErrWithWarnings()
+	}
+
+	newVal, readDiags := n.readDataSource(ctx, configVal)
+	diags = diags.Append(readDiags)
+	if diags.HasErrors() {
+		return nil, diags.ErrWithWarnings()
+	}
+
+	*n.State = &states.ResourceInstanceObject{
+		Value:  newVal,
+		Status: states.ObjectReady,
+	}
+
+	if err := ctx.Hook(func(h Hook) (HookAction, error) {
+		return h.PostApply(absAddr, states.CurrentGen, newVal, diags.Err())
+	}); err != nil {
+		diags = diags.Append(err)
+	}
+
+	return nil, diags.ErrWithWarnings()
+}

terraform/eval_read_data_plan.go

@@ -0,0 +1,173 @@
+package terraform
+
+import (
+	"fmt"
+	"log"
+
+	"github.com/zclconf/go-cty/cty"
+
+	"github.com/hashicorp/terraform/addrs"
+	"github.com/hashicorp/terraform/plans"
+	"github.com/hashicorp/terraform/plans/objchange"
+	"github.com/hashicorp/terraform/states"
+	"github.com/hashicorp/terraform/tfdiags"
+)
+
+// evalReadDataPlan is an EvalNode implementation that deals with the main part
+// of the data resource lifecycle: either actually reading from the data source
+// or generating a plan to do so.
+type evalReadDataPlan struct {
+	evalReadData
+
+	// dependsOn stores the list of transitive resource addresses that any
+	// configuration depends_on references may resolve to. This is used to
+	// determine if there are any changes that will force this data sources to
+	// be deferred to apply.
+	dependsOn []addrs.ConfigResource
+}
+
+func (n *evalReadDataPlan) Eval(ctx EvalContext) (interface{}, error) {
+	absAddr := n.Addr.Absolute(ctx.Path())
+
+	var diags tfdiags.Diagnostics
+	var configVal cty.Value
+
+	if n.ProviderSchema == nil || *n.ProviderSchema == nil {
+		return nil, fmt.Errorf("provider schema not available for %s", n.Addr)
+	}
+
+	config := *n.Config
+	providerSchema := *n.ProviderSchema
+	schema, _ := providerSchema.SchemaForResourceAddr(n.Addr.ContainingResource())
+	if schema == nil {
+		// Should be caught during validation, so we don't bother with a pretty error here
+		return nil, fmt.Errorf("provider %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type)
+	}
+
+	objTy := schema.ImpliedType()
+	priorVal := cty.NullVal(objTy)
+	if n.State != nil && *n.State != nil {
+		priorVal = (*n.State).Value
+	}
+
+	forEach, _ := evaluateForEachExpression(config.ForEach, ctx)
+	keyData := EvalDataForInstanceKey(n.Addr.Key, forEach)
+
+	var configDiags tfdiags.Diagnostics
+	configVal, _, configDiags = ctx.EvaluateBlock(config.Config, schema, nil, keyData)
+	diags = diags.Append(configDiags)
+	if configDiags.HasErrors() {
+		return nil, diags.ErrWithWarnings()
+	}
+
+	configKnown := configVal.IsWhollyKnown()
+	// If our configuration contains any unknown values, or we depend on any
+	// unknown values then we must defer the read to the apply phase by
+	// producing a "Read" change for this resource, and a placeholder value for
+	// it in the state.
+	if n.forcePlanRead(ctx) || !configKnown {
+		if configKnown {
+			log.Printf("[TRACE] evalReadDataPlan: %s configuration is fully known, but we're forcing a read plan to be created", absAddr)
+		} else {
+			log.Printf("[TRACE] evalReadDataPlan: %s configuration not fully known yet, so deferring to apply phase", absAddr)
+		}
+
+		proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal)
+
+		if err := ctx.Hook(func(h Hook) (HookAction, error) {
+			return h.PreDiff(absAddr, states.CurrentGen, priorVal, proposedNewVal)
+		}); err != nil {
+			diags = diags.Append(err)
+			return nil, diags.ErrWithWarnings()
+		}
+
+		// Apply detects that the data source will need to be read by the After
+		// value containing unknowns from PlanDataResourceObject.
+		*n.OutputChange = &plans.ResourceInstanceChange{
+			Addr:         absAddr,
+			ProviderAddr: n.ProviderAddr,
+			Change: plans.Change{
+				Action: plans.Read,
+				Before: priorVal,
+				After:  proposedNewVal,
+			},
+		}
+
+		*n.State = &states.ResourceInstanceObject{
+			Value:  cty.NullVal(objTy),
+			Status: states.ObjectPlanned,
+		}
+
+		if err := ctx.Hook(func(h Hook) (HookAction, error) {
+			return h.PostDiff(absAddr, states.CurrentGen, plans.Read, priorVal, proposedNewVal)
+		}); err != nil {
+			diags = diags.Append(err)
+		}
+
+		return nil, diags.ErrWithWarnings()
+	}
+
+	// If we have a stored state we may not need to re-read the data source.
+	// Check the config against the state to see if there are any difference.
+	if !priorVal.IsNull() {
+		// Applying the configuration to the prior state lets us see if there
+		// are any differences.
+		proposed := objchange.ProposedNewObject(schema, priorVal, configVal)
+		if proposed.Equals(priorVal).True() {
+			log.Printf("[TRACE] evalReadDataPlan: %s no change detected, using existing state", absAddr)
+			// state looks up to date, and must have been read during refresh
+			return nil, diags.ErrWithWarnings()
+		}
+	}
+
+	newVal, readDiags := n.readDataSource(ctx, configVal)
+	diags = diags.Append(readDiags)
+	if diags.HasErrors() {
+		return nil, diags.ErrWithWarnings()
+	}
+
+	// The returned value from ReadDataSource must be non-nil and known,
+	// which we store in the change. Apply will use the fact that the After
+	// value is wholly kown to save the state directly, rather than reading the
+	// data source again.
+	*n.OutputChange = &plans.ResourceInstanceChange{
+		Addr:         absAddr,
+		ProviderAddr: n.ProviderAddr,
+		Change: plans.Change{
+			Action: plans.Read,
+			Before: priorVal,
+			After:  newVal,
+		},
+	}
+
+	*n.State = &states.ResourceInstanceObject{
+		Value:  newVal,
+		Status: states.ObjectPlanned,
+	}
+
+	if err := ctx.Hook(func(h Hook) (HookAction, error) {
+		return h.PostDiff(absAddr, states.CurrentGen, plans.Update, priorVal, newVal)
+	}); err != nil {
+		return nil, err
+	}
+
+	return nil, diags.ErrWithWarnings()
+}
+
+// forcePlanRead determines if we need to override the usual behavior of
+// immediately reading from the data source where possible, instead forcing us
+// to generate a plan.
+func (n *evalReadDataPlan) forcePlanRead(ctx EvalContext) bool {
+	// Check and see if any depends_on dependencies have
+	// changes, since they won't show up as changes in the
+	// configuration.
+	changes := ctx.Changes()
+	for _, d := range n.dependsOn {
+		for _, change := range changes.GetChangesForConfigResource(d) {
+			if change != nil && change.Action != plans.NoOp {
+				return true
+			}
+		}
+	}
+	return false
+}

terraform/eval_state.go

@@ -187,6 +187,18 @@ func (n *EvalUpdateStateHook) Eval(ctx EvalContext) (interface{}, error) {
 	return nil, nil
 }
 
+// evalWriteEmptyState wraps EvalWriteState to specifically record an empty
+// state for a particular object.
+type evalWriteEmptyState struct {
+	EvalWriteState
+}
+
+func (n *evalWriteEmptyState) Eval(ctx EvalContext) (interface{}, error) {
+	var state *states.ResourceInstanceObject
+	n.State = &state
+	return n.EvalWriteState.Eval(ctx)
+}
+
 // EvalWriteState is an EvalNode implementation that saves the given object
 // as the current object for the selected resource instance.
 type EvalWriteState struct {

terraform/graph_builder_plan.go

@@ -150,6 +150,10 @@ func (b *PlanGraphBuilder) Steps() []GraphTransformer {
 		// have to connect again later for providers and so on.
 		&ReferenceTransformer{},
 
+		// Make sure data sources are aware of any depends_on from the
+		// configuration
+		&attachDataResourceDependenciesTransformer{},
+
 		// Add the node to fix the state count boundaries
 		&CountBoundaryTransformer{
 			Config: b.Config,

terraform/node_data_refresh.go

@@ -7,7 +7,6 @@ import (
 	"github.com/hashicorp/terraform/providers"
 	"github.com/hashicorp/terraform/states"
 	"github.com/hashicorp/terraform/tfdiags"
-	"github.com/zclconf/go-cty/cty"
 )
 
 type nodeExpandRefreshableDataResource struct {
@@ -199,7 +198,6 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode {
 	var providerSchema *ProviderSchema
 	var change *plans.ResourceInstanceChange
 	var state *states.ResourceInstanceObject
-	var configVal cty.Value
 
 	return &EvalSequence{
 		Nodes: []EvalNode{
@@ -209,21 +207,18 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode {
 				Schema: &providerSchema,
 			},
 
-			// Always destroy the existing state first, since we must
+			&EvalReadState{
-			// make sure that values from a previous read will not
-			// get interpolated if we end up needing to defer our
-			// loading until apply time.
-			&EvalWriteState{
 				Addr:           addr.Resource,
-				ProviderAddr:   n.ResolvedProvider,
+				Provider:       &provider,
-				State:          &state, // a pointer to nil, here
 				ProviderSchema: &providerSchema,
+				Output:         &state,
 			},
 
-			// EvalReadData will _attempt_ to read the data source, but may
+			// EvalReadDataRefresh will _attempt_ to read the data source, but
-			// generate an incomplete planned object if the configuration
+			// may generate an incomplete planned object if the configuration
 			// includes values that won't be known until apply.
-			&EvalReadData{
+			&evalReadDataRefresh{
+				evalReadData{
 					Addr:           addr.Resource,
 					Config:         n.Config,
 					Provider:       &provider,
@@ -231,19 +226,14 @@ func (n *NodeRefreshableDataResourceInstance) EvalTree() EvalNode {
 					ProviderMetas:  n.ProviderMetas,
 					ProviderSchema: &providerSchema,
 					OutputChange:   &change,
-				OutputConfigValue: &configVal,
+					State:          &state,
-				OutputState:       &state,
+				},
-				// If the config explicitly has a depends_on for this data
-				// source, assume the intention is to prevent refreshing ahead
-				// of that dependency, and therefore we need to deal with this
-				// resource during the apply phase. We do that by forcing this
-				// read to result in a plan.
-				ForcePlanRead: len(n.Config.DependsOn) > 0,
 			},
 
 			&EvalIf{
 				If: func(ctx EvalContext) (bool, error) {
-					return (*state).Status != states.ObjectPlanned, nil
+					return change == nil, nil
+
 				},
 				Then: &EvalSequence{
 					Nodes: []EvalNode{

terraform/node_resource_abstract.go

@@ -58,7 +58,11 @@ type NodeAbstractResource struct {
 
 	ProvisionerSchemas map[string]*configschema.Block
 
-	Targets []addrs.Targetable // Set from GraphNodeTargetable
+	// Set from GraphNodeTargetable
+	Targets []addrs.Targetable
+
+	// Set from GraphNodeDependsOn
+	dependsOn []addrs.ConfigResource
 
 	// The address of the provider this resource will use
 	ResolvedProvider addrs.AbsProviderConfig
@@ -75,6 +79,7 @@ var (
 	_ GraphNodeAttachProvisionerSchema    = (*NodeAbstractResource)(nil)
 	_ GraphNodeAttachProviderMetaConfigs  = (*NodeAbstractResource)(nil)
 	_ GraphNodeTargetable                 = (*NodeAbstractResource)(nil)
+	_ graphNodeAttachResourceDependencies = (*NodeAbstractResource)(nil)
 	_ dag.GraphNodeDotter                 = (*NodeAbstractResource)(nil)
 )
 
@@ -175,18 +180,7 @@ func (n *NodeAbstractResource) References() []*addrs.Reference {
 	if c := n.Config; c != nil {
 		var result []*addrs.Reference
 
-		for _, traversal := range c.DependsOn {
+		result = append(result, n.DependsOn()...)
-			ref, diags := addrs.ParseRef(traversal)
-			if diags.HasErrors() {
-				// We ignore this here, because this isn't a suitable place to return
-				// errors. This situation should be caught and rejected during
-				// validation.
-				log.Printf("[ERROR] Can't parse %#v from depends_on as reference: %s", traversal, diags.Err())
-				continue
-			}
-
-			result = append(result, ref)
-		}
 
 		if n.Schema == nil {
 			// Should never happens, but we'll log if it does so that we can
@@ -230,6 +224,26 @@ func (n *NodeAbstractResource) References() []*addrs.Reference {
 	return nil
 }
 
+func (n *NodeAbstractResource) DependsOn() []*addrs.Reference {
+	var result []*addrs.Reference
+	if c := n.Config; c != nil {
+
+		for _, traversal := range c.DependsOn {
+			ref, diags := addrs.ParseRef(traversal)
+			if diags.HasErrors() {
+				// We ignore this here, because this isn't a suitable place to return
+				// errors. This situation should be caught and rejected during
+				// validation.
+				log.Printf("[ERROR] Can't parse %#v from depends_on as reference: %s", traversal, diags.Err())
+				continue
+			}
+
+			result = append(result, ref)
+		}
+	}
+	return result
+}
+
 // GraphNodeReferencer
 func (n *NodeAbstractResourceInstance) References() []*addrs.Reference {
 	// If we have a configuration attached then we'll delegate to our
@@ -382,6 +396,11 @@ func (n *NodeAbstractResource) SetTargets(targets []addrs.Targetable) {
 	n.Targets = targets
 }
 
+// graphNodeAttachResourceDependencies
+func (n *NodeAbstractResource) AttachResourceDependencies(deps []addrs.ConfigResource) {
+	n.dependsOn = deps
+}
+
 // GraphNodeAttachResourceState
 func (n *NodeAbstractResourceInstance) AttachResourceState(s *states.Resource) {
 	n.ResourceState = s

terraform/node_resource_apply_instance.go

@@ -3,8 +3,6 @@ package terraform
 import (
 	"fmt"
 
-	"github.com/zclconf/go-cty/cty"
-
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/plans"
@@ -185,15 +183,17 @@ func (n *NodeApplyableResourceInstance) evalTreeDataResource(addr addrs.AbsResou
 			// In this particular call to EvalReadData we include our planned
 			// change, which signals that we expect this read to complete fully
 			// with no unknown values; it'll produce an error if not.
-			&EvalReadData{
+			&evalReadDataApply{
+				evalReadData{
 					Addr:           addr.Resource,
 					Config:         n.Config,
-				Planned:        &change, // setting this indicates that the result must be complete
+					Planned:        &change,
 					Provider:       &provider,
 					ProviderAddr:   n.ResolvedProvider,
 					ProviderMetas:  n.ProviderMetas,
 					ProviderSchema: &providerSchema,
-				OutputState:    &state,
+					State:          &state,
+				},
 			},
 
 			&EvalWriteState{
@@ -226,7 +226,6 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe
 	var err error
 	var createNew bool
 	var createBeforeDestroyEnabled bool
-	var configVal cty.Value
 	var deposedKey states.DeposedKey
 
 	return &EvalSequence{
@@ -304,7 +303,6 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe
 				State:          &state,
 				PreviousDiff:   &diff,
 				OutputChange:   &diffApply,
-				OutputValue:    &configVal,
 				OutputState:    &state,
 			},
 

terraform/node_resource_destroy.go

@@ -234,22 +234,16 @@ func (n *NodeDestroyResourceInstance) EvalTree() EvalNode {
 					},
 				},
 
-				// Make sure we handle data sources properly.
+				// Managed resources need to be destroyed, while data sources
+				// are only removed from state.
 				&EvalIf{
 					If: func(ctx EvalContext) (bool, error) {
-						return addr.Resource.Resource.Mode == addrs.DataResourceMode, nil
+						return addr.Resource.Resource.Mode == addrs.ManagedResourceMode, nil
 					},
 
-					Then: &EvalReadDataApply{
+					Then: &EvalSequence{
-						Addr:           addr.Resource,
+						Nodes: []EvalNode{
-						Config:         n.Config,
+							&EvalApply{
-						Change:         &changeApply,
-						Provider:       &provider,
-						ProviderAddr:   n.ResolvedProvider,
-						ProviderSchema: &providerSchema,
-						Output:         &state,
-					},
-					Else: &EvalApply{
 								Addr:           addr.Resource,
 								Config:         nil, // No configuration because we are destroying
 								State:          &state,
@@ -261,13 +255,23 @@ func (n *NodeDestroyResourceInstance) EvalTree() EvalNode {
 								Output:         &state,
 								Error:          &err,
 							},
-				},
 							&EvalWriteState{
 								Addr:           addr.Resource,
 								ProviderAddr:   n.ResolvedProvider,
 								ProviderSchema: &providerSchema,
 								State:          &state,
 							},
+						},
+					},
+					Else: &evalWriteEmptyState{
+						EvalWriteState{
+							Addr:           addr.Resource,
+							ProviderAddr:   n.ResolvedProvider,
+							ProviderSchema: &providerSchema,
+						},
+					},
+				},
+
 				&EvalApplyPost{
 					Addr:  addr.Resource,
 					State: &state,

terraform/node_resource_plan.go

@@ -218,6 +218,7 @@ func (n *NodePlannableResource) DynamicExpand(ctx EvalContext) (*Graph, error) {
 		a.Schema = n.Schema
 		a.ProvisionerSchemas = n.ProvisionerSchemas
 		a.ProviderMetas = n.ProviderMetas
+		a.dependsOn = n.dependsOn
 
 		return &NodePlannableResourceInstance{
 			NodeAbstractResourceInstance: a,

terraform/node_resource_plan_instance.go

@@ -8,7 +8,6 @@ import (
 	"github.com/hashicorp/terraform/states"
 
 	"github.com/hashicorp/terraform/addrs"
-	"github.com/zclconf/go-cty/cty"
 )
 
 // NodePlannableResourceInstance represents a _single_ resource
@@ -51,7 +50,6 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou
 	var providerSchema *ProviderSchema
 	var change *plans.ResourceInstanceChange
 	var state *states.ResourceInstanceObject
-	var configVal cty.Value
 
 	return &EvalSequence{
 		Nodes: []EvalNode{
@@ -69,59 +67,24 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou
 				Output: &state,
 			},
 
-			// If we already have a non-planned state then we already dealt
-			// with this during the refresh walk and so we have nothing to do
-			// here.
-			&EvalIf{
-				If: func(ctx EvalContext) (bool, error) {
-					depChanges := false
-
-					// Check and see if any of our dependencies have changes.
-					changes := ctx.Changes()
-					for _, d := range n.References() {
-						ri, ok := d.Subject.(addrs.ResourceInstance)
-						if !ok {
-							continue
-						}
-						change := changes.GetResourceInstanceChange(ri.Absolute(ctx.Path()), states.CurrentGen)
-						if change != nil && change.Action != plans.NoOp {
-							depChanges = true
-							break
-						}
-					}
-
-					refreshed := state != nil && state.Status != states.ObjectPlanned
-
-					// If there are no dependency changes, and it's not a forced
-					// read because we there was no Refresh, then we don't need
-					// to re-read. If any dependencies have changes, it means
-					// our config may also have changes and we need to Read the
-					// data source again.
-					if !depChanges && refreshed {
-						return false, EvalEarlyExitError{}
-					}
-					return true, nil
-				},
-				Then: EvalNoop{},
-			},
-
 			&EvalValidateSelfRef{
 				Addr:           addr.Resource,
 				Config:         config.Config,
 				ProviderSchema: &providerSchema,
 			},
 
-			&EvalReadData{
+			&evalReadDataPlan{
+				evalReadData: evalReadData{
 					Addr:           addr.Resource,
 					Config:         n.Config,
 					Provider:       &provider,
 					ProviderAddr:   n.ResolvedProvider,
 					ProviderMetas:  n.ProviderMetas,
 					ProviderSchema: &providerSchema,
-				ForcePlanRead:  true, // _always_ produce a Read change, even if the config seems ready
 					OutputChange:   &change,
-				OutputValue:    &configVal,
+					State:          &state,
-				OutputState:    &state,
+				},
+				dependsOn: n.dependsOn,
 			},
 
 			&EvalWriteState{
@@ -159,7 +122,6 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe
 				Addr:           addr.Resource,
 				Provider:       &provider,
 				ProviderSchema: &providerSchema,
-
 				Output:         &state,
 			},
 

terraform/testdata/apply-data-depends-on/main.tf

@@ -1,8 +0,0 @@
-resource "null_instance" "write" {
-	foo = "attribute"
-}
-
-data "null_data_source" "read" {
-	foo = ""
-	depends_on = ["null_instance.write"]
-}

terraform/transform_reference.go

@@ -7,11 +7,9 @@ import (
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/terraform/addrs"
-	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/configs/configschema"
 	"github.com/hashicorp/terraform/dag"
 	"github.com/hashicorp/terraform/lang"
-	"github.com/hashicorp/terraform/states"
 )
 
 // GraphNodeReferenceable must be implemented by any node that represents
@@ -45,6 +43,22 @@ type GraphNodeAttachDependencies interface {
 	AttachDependencies([]addrs.ConfigResource)
 }
 
+// graphNodeAttachResourceDependencies records all resources that are transitively
+// referenced through depends_on in the configuration. This is used by data
+// resources to determine if they can be read during the plan, or if they need
+// to be further delayed until apply.
+// We can only use an addrs.ConfigResource address here, because modules are
+// not yet expended in the graph. While this will cause some extra data
+// resources to show in the plan when their depends_on references may be in
+// unrelated module instances, the fact that it only happens when there are any
+// resource updates pending means we can still avoid the problem of the
+// "perpetual diff"
+type graphNodeAttachResourceDependencies interface {
+	GraphNodeConfigResource
+	AttachResourceDependencies([]addrs.ConfigResource)
+	DependsOn() []*addrs.Reference
+}
+
 // GraphNodeReferenceOutside is an interface that can optionally be implemented.
 // A node that implements it can specify that its own referenceable addresses
 // and/or the addresses it references are in a different module than the
@@ -106,18 +120,81 @@ func (t *ReferenceTransformer) Transform(g *Graph) error {
 	return nil
 }
 
+type depMap map[string]addrs.ConfigResource
+
+// addDep adds the vertex if it represents a resource in the
+// graph.
+func (m depMap) add(v dag.Vertex) {
+	// we're only concerned with resources which may have changes that
+	// need to be applied.
+	switch v := v.(type) {
+	case GraphNodeResourceInstance:
+		instAddr := v.ResourceInstanceAddr()
+		addr := instAddr.ContainingResource().Config()
+		m[addr.String()] = addr
+	case GraphNodeConfigResource:
+		addr := v.ResourceAddr()
+		m[addr.String()] = addr
+	}
+}
+
+// attachDataResourceDependenciesTransformer records all resources transitively referenced
+// through a configuration depends_on.
+type attachDataResourceDependenciesTransformer struct {
+}
+
+func (t attachDataResourceDependenciesTransformer) Transform(g *Graph) error {
+	// First we need to make a map of referenceable addresses to their vertices.
+	// This is very similar to what's done in ReferenceTransformer, but we keep
+	// implementation separate as they may need to change independently.
+	vertices := g.Vertices()
+	refMap := NewReferenceMap(vertices)
+
+	for _, v := range vertices {
+		depender, ok := v.(graphNodeAttachResourceDependencies)
+		if !ok {
+			continue
+		}
+		selfAddr := depender.ResourceAddr()
+
+		// Only data need to attach depends_on, so they can determine if they
+		// are eligible to be read during plan.
+		if selfAddr.Resource.Mode != addrs.DataResourceMode {
+			continue
+		}
+
+		// depMap will only add resource references and dedupe
+		m := make(depMap)
+
+		for _, dep := range refMap.DependsOn(v) {
+			// any the dependency
+			m.add(dep)
+			// and check any ancestors
+			ans, _ := g.Ancestors(dep)
+			for _, v := range ans {
+				m.add(v)
+			}
+		}
+
+		deps := make([]addrs.ConfigResource, 0, len(m))
+		for _, d := range m {
+			deps = append(deps, d)
+		}
+
+		log.Printf("[TRACE] AttachDependsOnTransformer: %s depends on %s", depender.ResourceAddr(), deps)
+		depender.AttachResourceDependencies(deps)
+	}
+
+	return nil
+}
+
 // AttachDependenciesTransformer records all resource dependencies for each
 // instance, and attaches the addresses to the node itself. Managed resource
 // will record these in the state for proper ordering of destroy operations.
 type AttachDependenciesTransformer struct {
-	Config  *configs.Config
-	State   *states.State
-	Schemas *Schemas
 }
 
 func (t AttachDependenciesTransformer) Transform(g *Graph) error {
-	// FIXME: this is only working with ResourceConfigAddr for now
-
 	for _, v := range g.Vertices() {
 		attacher, ok := v.(GraphNodeAttachDependencies)
 		if !ok {
@@ -228,19 +305,13 @@ func (t *PruneUnusedValuesTransformer) Transform(g *Graph) error {
 }
 
 // ReferenceMap is a structure that can be used to efficiently check
-// for references on a graph.
+// for references on a graph, mapping internal reference keys (as produced by
-type ReferenceMap struct {
+// the mapKey method) to one or more vertices that are identified by each key.
-	// vertices is a map from internal reference keys (as produced by the
+type ReferenceMap map[string][]dag.Vertex
-	// mapKey method) to one or more vertices that are identified by each key.
-	//
-	// A particular reference key might actually identify multiple vertices,
-	// e.g. in situations where one object is contained inside another.
-	vertices map[string][]dag.Vertex
-}
 
 // References returns the set of vertices that the given vertex refers to,
 // and any referenced addresses that do not have corresponding vertices.
-func (m *ReferenceMap) References(v dag.Vertex) []dag.Vertex {
+func (m ReferenceMap) References(v dag.Vertex) []dag.Vertex {
 	rn, ok := v.(GraphNodeReferencer)
 	if !ok {
 		return nil
@@ -252,7 +323,7 @@ func (m *ReferenceMap) References(v dag.Vertex) []dag.Vertex {
 		subject := ref.Subject
 
 		key := m.referenceMapKey(v, subject)
-		if _, exists := m.vertices[key]; !exists {
+		if _, exists := m[key]; !exists {
 			// If what we were looking for was a ResourceInstance then we
 			// might be in a resource-oriented graph rather than an
 			// instance-oriented graph, and so we'll see if we have the
@@ -270,7 +341,38 @@ func (m *ReferenceMap) References(v dag.Vertex) []dag.Vertex {
 			}
 			key = m.referenceMapKey(v, subject)
 		}
-		vertices := m.vertices[key]
+		vertices := m[key]
+		for _, rv := range vertices {
+			// don't include self-references
+			if rv == v {
+				continue
+			}
+			matches = append(matches, rv)
+		}
+	}
+
+	return matches
+}
+
+// DependsOn returns the set of vertices that the given vertex refers to from
+// the configured depends_on.
+func (m ReferenceMap) DependsOn(v dag.Vertex) []dag.Vertex {
+	depender, ok := v.(graphNodeAttachResourceDependencies)
+	if !ok {
+		return nil
+	}
+
+	var matches []dag.Vertex
+
+	for _, ref := range depender.DependsOn() {
+		subject := ref.Subject
+
+		key := m.referenceMapKey(v, subject)
+		vertices, ok := m[key]
+		if !ok {
+			log.Printf("[WARN] DependOn: reference not found: %q", subject)
+			continue
+		}
 		for _, rv := range vertices {
 			// don't include self-references
 			if rv == v {
@@ -351,11 +453,9 @@ func (m *ReferenceMap) referenceMapKey(referrer dag.Vertex, addr addrs.Reference
 
 // NewReferenceMap is used to create a new reference map for the
 // given set of vertices.
-func NewReferenceMap(vs []dag.Vertex) *ReferenceMap {
+func NewReferenceMap(vs []dag.Vertex) ReferenceMap {
-	var m ReferenceMap
-
 	// Build the lookup table
-	vertices := make(map[string][]dag.Vertex)
+	m := make(ReferenceMap)
 	for _, v := range vs {
 		// We're only looking for referenceable nodes
 		rn, ok := v.(GraphNodeReferenceable)
@@ -368,12 +468,11 @@ func NewReferenceMap(vs []dag.Vertex) *ReferenceMap {
 		// Go through and cache them
 		for _, addr := range rn.ReferenceableAddrs() {
 			key := m.mapKey(path, addr)
-			vertices[key] = append(vertices[key], v)
+			m[key] = append(m[key], v)
 		}
 	}
 
-	m.vertices = vertices
+	return m
-	return &m
 }
 
 // ReferencesFromConfig returns the references that a configuration has