core: Reinstaint instance tainting, but without mutating objects

Our previous mechanism for dealing with tainting relied on directly
mutating the InstanceState object to mark it as such. In our new state
models we consider the instance objects to be immutable by convention, and
so we frequently copy them. As a result, the taint flagging was no longer
making it all the way through the apply evaluation process.

Here we now implement tainting as a separate step in the evaluation
process, creating a copy of the object with a tainted status if there were
any errors during creation.

This introduces a new behavior where any provider-level errors during
creation will also cause an instance to be marked as tainted if any object
is returned at all. Create-time errors _normally_ result in no object at
all, but the provider might return an object if the failure occurred at
a subsequent step of a multi-step creation process and so left behind a
remote object that needs to be cleaned up on a future run.

states/instance_object.go

@@ -104,3 +104,17 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res
 		Dependencies:  o.Dependencies,
 	}, nil
 }
+
+// AsTainted returns a deep copy of the receiver with the status updated to
+// ObjectTainted.
+func (o *ResourceInstanceObject) AsTainted() *ResourceInstanceObject {
+	if o == nil {
+		// A nil object can't be tainted, but we'll allow this anyway to
+		// avoid a crash, since we presumably intend to eventually record
+		// the object has having been deleted anyway.
+		return nil
+	}
+	ret := o.DeepCopy()
+	ret.Status = ObjectTainted
+	return ret
+}

terraform/eval_apply.go

@@ -171,7 +171,7 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
 	var newState *states.ResourceInstanceObject
 	if !newVal.IsNull() { // null value indicates that the object is deleted, so we won't set a new state in that case
 		newState = &states.ResourceInstanceObject{
-			Status:       states.ObjectReady, // TODO: Consider marking as tainted if the provider returned errors?
+			Status:       states.ObjectReady,
 			Value:        newVal,
 			Private:      resp.Private,
 			Dependencies: n.Dependencies, // Should be populated by the caller from the StateDependencies method on the resource instance node
@@ -262,6 +262,57 @@ func (n *EvalApplyPost) Eval(ctx EvalContext) (interface{}, error) {
 	return nil, *n.Error
 }
 
+// EvalMaybeTainted is an EvalNode that takes the planned change, new value,
+// and possible error from an apply operation and produces a new instance
+// object marked as tainted if it appears that a create operation has failed.
+//
+// This EvalNode never returns an error, to ensure that a subsequent EvalNode
+// can still record the possibly-tainted object in the state.
+type EvalMaybeTainted struct {
+	Addr   addrs.ResourceInstance
+	Gen    states.Generation
+	Change **plans.ResourceInstanceChange
+	State  **states.ResourceInstanceObject
+	Error  *error
+
+	// If StateOutput is not nil, its referent will be assigned either the same
+	// pointer as State or a new object with its status set as Tainted,
+	// depending on whether an error is given and if this was a create action.
+	StateOutput **states.ResourceInstanceObject
+}
+
+// TODO: test
+func (n *EvalMaybeTainted) Eval(ctx EvalContext) (interface{}, error) {
+	state := *n.State
+	change := *n.Change
+	err := *n.Error
+
+	if state != nil && state.Status == states.ObjectTainted {
+		log.Printf("[TRACE] EvalMaybeTainted: %s was already tainted, so nothing to do", n.Addr.Absolute(ctx.Path()))
+		return nil, nil
+	}
+
+	if n.StateOutput != nil {
+		if err != nil && change.Action == plans.Create {
+			// If there are errors during a _create_ then the object is
+			// in an undefined state, and so we'll mark it as tainted so
+			// we can try again on the next run.
+			//
+			// We don't do this for other change actions because errors
+			// during updates will often not change the remote object at all.
+			// If there _were_ changes prior to the error, it's the provider's
+			// responsibility to record the effect of those changes in the
+			// object value it returned.
+			log.Printf("[TRACE] EvalMaybeTainted: %s encountered an error during creation, so it is now marked as tainted", n.Addr.Absolute(ctx.Path()))
+			*n.StateOutput = state.AsTainted()
+		} else {
+			*n.StateOutput = state
+		}
+	}
+
+	return nil, nil
+}
+
 // resourceHasUserVisibleApply returns true if the given resource is one where
 // apply actions should be exposed to the user.
 //
@@ -300,9 +351,15 @@ func (n *EvalApplyProvisioners) Eval(ctx EvalContext) (interface{}, error) {
 		log.Printf("[TRACE] EvalApplyProvisioners: %s has no state, so skipping provisioners", n.Addr)
 		return nil, nil
 	}
-
 	if n.CreateNew != nil && !*n.CreateNew {
 		// If we're not creating a new resource, then don't run provisioners
+		log.Printf("[TRACE] EvalApplyProvisioners: %s is not freshly-created, so no provisioning is required", n.Addr)
+		return nil, nil
+	}
+	if state.Status == states.ObjectTainted {
+		// No point in provisioning an object that is already tainted, since
+		// it's going to get recreated on the next apply anyway.
+		log.Printf("[TRACE] EvalApplyProvisioners: %s is tainted, so skipping provisioning", n.Addr)
 		return nil, nil
 	}
 
@@ -312,14 +369,7 @@ func (n *EvalApplyProvisioners) Eval(ctx EvalContext) (interface{}, error) {
 		return nil, nil
 	}
 
-	// taint tells us whether to enable tainting.
-	taint := n.When == configs.ProvisionerWhenCreate
-
 	if n.Error != nil && *n.Error != nil {
-		if taint {
-			state.Status = states.ObjectTainted
-		}
-
 		// We're already tainted, so just return out
 		return nil, nil
 	}
@@ -338,12 +388,13 @@ func (n *EvalApplyProvisioners) Eval(ctx EvalContext) (interface{}, error) {
 	// if we have one, otherwise we just output it.
 	err := n.apply(ctx, provs)
 	if err != nil {
-		if taint {
-			state.Status = states.ObjectTainted
-		}
-
 		*n.Error = multierror.Append(*n.Error, err)
+		if n.Error == nil {
 			return nil, err
+		} else {
+			log.Printf("[TRACE] EvalApplyProvisioners: %s provisioning failed, but we will continue anyway at the caller's request", absAddr)
+			return nil, nil
+		}
 	}
 
 	{

terraform/node_resource_apply_instance.go

@@ -342,6 +342,13 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe
 				Error:          &err,
 				CreateNew:      &createNew,
 			},
+			&EvalMaybeTainted{
+				Addr:        addr.Resource,
+				State:       &state,
+				Change:      &diffApply,
+				Error:       &err,
+				StateOutput: &state,
+			},
 			&EvalWriteState{
 				Addr:           addr.Resource,
 				ProviderAddr:   n.ResolvedProvider,
@@ -350,12 +357,19 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe
 			},
 			&EvalApplyProvisioners{
 				Addr:           addr.Resource,
-				State:          &state,
+				State:          &state, // EvalApplyProvisioners will skip if already tainted
 				ResourceConfig: n.Config,
 				CreateNew:      &createNew,
 				Error:          &err,
 				When:           configs.ProvisionerWhenCreate,
 			},
+			&EvalMaybeTainted{
+				Addr:        addr.Resource,
+				State:       &state,
+				Change:      &diffApply,
+				Error:       &err,
+				StateOutput: &state,
+			},
 			&EvalWriteState{
 				Addr:           addr.Resource,
 				ProviderAddr:   n.ResolvedProvider,