read data sources during plan

In order to udpate data sources correctly when their configuration
changes, they need to be evaluated during plan. Since the plan working
state isn't saved, store any data source reads as plan changes to be
applied later. This is currently abusing the Update plan action to
indicate that the data source was read and needs to be applied to state.
We can possibly add a Store action for data sources if this approach
works out.  The Read action still indicates that the data source was
deferred to the Apply phase.

We also fully handle any data source depends_on changes. Now that all
the transitive resource dependencies are known at the time of
evaluation, we can check the plan to determine if there are any changes
in the dependencies and selectively defer reading the data source.

terraform/eval_read_data.go

@@ -39,7 +39,7 @@ type EvalReadData struct {
 	// _always_ generate a plan. This is used during the plan walk, since we
 	// mustn't actually apply anything there. (The resulting state doesn't
 	// get persisted)
-	ForcePlanRead bool
+	ForcePlanRead *bool
 
 	// The result from this EvalNode has a few different possibilities
 	// depending on the input:
@@ -57,6 +57,10 @@ type EvalReadData struct {
 	//   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.
+	//
+	// FIXME: these fields are a mess. OutputValue is getting the config passed
+	// in, OutputState is passed in as well, and OuputValue is replaced with
+	// the state value which goes in OutputState.
 	OutputChange      **plans.ResourceInstanceChange
 	OutputValue       *cty.Value
 	OutputConfigValue *cty.Value
@@ -64,15 +68,24 @@ type EvalReadData struct {
 }
 
 func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
+	state := *n.OutputState
 	absAddr := n.Addr.Absolute(ctx.Path())
-	log.Printf("[TRACE] EvalReadData: working on %s", absAddr)
+
+	var planned *plans.ResourceInstanceChange
+	if n.Planned != nil {
+		planned = *n.Planned
+	}
+
+	forcePlanRead := false
+	if n.ForcePlanRead != nil {
+		forcePlanRead = *n.ForcePlanRead
+	}
 
 	if n.ProviderSchema == nil || *n.ProviderSchema == nil {
 		return nil, fmt.Errorf("provider schema not available for %s", n.Addr)
 	}
 
 	var diags tfdiags.Diagnostics
-	var change *plans.ResourceInstanceChange
 	var configVal cty.Value
 
 	// TODO: Do we need to handle Delete changes here? EvalReadDataDiff and
@@ -90,11 +103,17 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 		return nil, fmt.Errorf("provider %q does not support data source %q", n.ProviderAddr.Provider.String(), n.Addr.Resource.Type)
 	}
 
-	// We'll always start by evaluating the configuration. What we do after
+	// While data source are read-only, and don't necessarily use the prior
-	// that will depend on the evaluation result along with what other inputs
+	// state, we record it here and use it to determine if we have a change or
-	// we were given.
+	// not.  If we needed to read a new value, but it still matches the
+	// previous state, then we can record a NoNop change. If the states don't
+	// match then we record a Read change so that the new value is applied to
+	// the state.
 	objTy := schema.ImpliedType()
-	priorVal := cty.NullVal(objTy) // for data resources, prior is always null because we start fresh every time
+	priorVal := cty.NullVal(objTy)
+	if state != nil {
+		priorVal = state.Value
+	}
 
 	forEach, _ := evaluateForEachExpression(n.Config.ForEach, ctx)
 	keyData := EvalDataForInstanceKey(n.Addr.Key, forEach)
@@ -130,20 +149,21 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 
 	proposedNewVal := objchange.PlannedDataResourceObject(schema, configVal)
 
+	configKnown := configVal.IsWhollyKnown()
 	// 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 forcePlanRead || !configKnown {
 		// 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 {
+		if 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 {
+		if configKnown {
 			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)
@@ -156,7 +176,7 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 			return nil, err
 		}
 
-		change = &plans.ResourceInstanceChange{
+		change := &plans.ResourceInstanceChange{
 			Addr:         absAddr,
 			ProviderAddr: n.ProviderAddr,
 			Change: plans.Change{
@@ -182,10 +202,11 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 		if n.OutputConfigValue != nil {
 			*n.OutputConfigValue = configVal
 		}
+
 		if n.OutputState != nil {
 			state := &states.ResourceInstanceObject{
-				Value:  change.After,
+				Value:  cty.NullVal(objTy),
-				Status: states.ObjectPlanned, // because the partial value in the plan must be used for now
+				Status: states.ObjectPlanned,
 			}
 			*n.OutputState = state
 		}
@@ -193,15 +214,49 @@ func (n *EvalReadData) Eval(ctx EvalContext) (interface{}, error) {
 		return nil, diags.ErrWithWarnings()
 	}
 
-	if n.Planned != nil && *n.Planned != nil && (*n.Planned).Action != plans.Read {
+	if planned != nil {
-		// If any other action gets in here then that's always a bug; this
+		if !(planned.Action == plans.Read || planned.Action == plans.Update) {
-		// EvalNode only deals with reading.
+			// If any other action gets in here then that's always a bug; this
-		return nil, fmt.Errorf(
+			// EvalNode only deals with reading.
-			"invalid action %s for %s: only Read is supported (this is a bug in Terraform; please report it!)",
+			return nil, fmt.Errorf(
-			(*n.Planned).Action, absAddr,
+				"invalid action %s for %s: only Read or Update is supported (this is a bug in Terraform; please report it!)",
-		)
+				planned.Action, absAddr,
+			)
+		}
+
+		// we have a change and it is complete, which means we read the data
+		// source during plan.
+		if planned.Action == plans.Update {
+			state = &states.ResourceInstanceObject{
+				Value:  planned.After,
+				Status: states.ObjectReady,
+			}
+
+			err := ctx.Hook(func(h Hook) (HookAction, error) {
+				return h.PostRefresh(absAddr, states.CurrentGen, planned.Before, planned.After)
+			})
+			if err != nil {
+				return nil, err
+			}
+
+			if n.OutputChange != nil {
+				*n.OutputChange = planned
+			}
+			if n.OutputValue != nil {
+				*n.OutputValue = planned.After
+			}
+			if n.OutputConfigValue != nil {
+				*n.OutputConfigValue = configVal
+			}
+			if n.OutputState != nil {
+				*n.OutputState = state
+			}
+			return nil, diags.ErrWithWarnings()
+		}
 	}
 
+	var change *plans.ResourceInstanceChange
+
 	log.Printf("[TRACE] Re-validating config for %s", absAddr)
 	validateResp := provider.ValidateDataSourceConfig(
 		providers.ValidateDataSourceConfigRequest{
@@ -266,7 +321,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,19 +341,24 @@ 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
+	action := plans.NoOp
-	// we'll produce a NoOp one anyway to preserve the usual flow of the
+	if !newVal.IsNull() && newVal.IsKnown() && newVal.Equals(priorVal).False() {
-	// plan phase and allow it to produce a complete plan.
+		// FIXME: for now we are abusing Update to mean "apply this new value"
+		action = plans.Update
+	}
+
+	// Produce a change regardless of the outcome.
 	change = &plans.ResourceInstanceChange{
 		Addr:         absAddr,
 		ProviderAddr: n.ProviderAddr,
 		Change: plans.Change{
-			Action: plans.NoOp,
+			Action: action,
-			Before: newVal,
+			Before: priorVal,
 			After:  newVal,
 		},
 	}
-	state := &states.ResourceInstanceObject{
+
+	state = &states.ResourceInstanceObject{
 		Value:  change.After,
 		Status: states.ObjectReady, // because we completed the read from the provider
 	}

terraform/node_resource_plan_instance.go

@@ -53,6 +53,8 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou
 	var state *states.ResourceInstanceObject
 	var configVal cty.Value
 
+	forcePlanRead := new(bool)
+
 	return &EvalSequence{
 		Nodes: []EvalNode{
 			&EvalGetProvider{
@@ -76,30 +78,22 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou
 				If: func(ctx EvalContext) (bool, error) {
 					depChanges := false
 
-					// Check and see if any of our dependencies have changes.
+					// 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.References() {
+					depChanges = func() bool {
-						ri, ok := d.Subject.(addrs.ResourceInstance)
+						for _, d := range n.dependsOn {
-						if !ok {
+							for _, change := range changes.GetConfigResourceChanges(d) {
-							continue
+								if change != nil && change.Action != plans.NoOp {
+									return true
+								}
+							}
 						}
-						change := changes.GetResourceInstanceChange(ri.Absolute(ctx.Path()), states.CurrentGen)
+						return false
-						if change != nil && change.Action != plans.NoOp {
+					}()
-							depChanges = true
-							break
-						}
-					}
 
-					refreshed := state != nil && state.Status != states.ObjectPlanned
+					*forcePlanRead = depChanges
-
-					// 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{},
@@ -118,7 +112,7 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou
 				ProviderAddr:   n.ResolvedProvider,
 				ProviderMetas:  n.ProviderMetas,
 				ProviderSchema: &providerSchema,
-				ForcePlanRead:  true, // _always_ produce a Read change, even if the config seems ready
+				ForcePlanRead:  forcePlanRead,
 				OutputChange:   &change,
 				OutputValue:    &configVal,
 				OutputState:    &state,