Use Outputs instead of merged Inputs+Outputs (#2659)

Fixes #2650.

We have historically relied on merging inputs and outputs in several places in the engine. This used to be necessary, as discussed in #2650 (comment), but our core engine model has moved away from depending on this. However, we still have a couple places we do this merge, and those places have triggered several severe issues recently in subtle cases.

We believe that this merging should no longer be needed for a correct interpretation of the current engine model, and indeed that doing the merge actively violates the contract with providers. In this PR we remove the remaining places where this input + output merge was being done. In all three cases, we use just the Outputs, which for most providers will already include the same values as the inputs - but correctly as determined by the provider itself.

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ### Improvements
 
+- Fix an engine bug that could lead to incorrect interpretation of the previous state of a resource leading to 
+  unexpected Update, Replace or Delete operations being scheduled. [#2650]https://github.com/pulumi/pulumi/issues/2650)
+
 ## 0.17.7 (Released April 17, 2019)
 
 ### Improvements

pkg/resource/deploy/source_eval.go

@@ -728,7 +728,6 @@ func (rm *resmon) RegisterResource(ctx context.Context,
 	}
 
 	state := result.State
-	props = state.All()
 	stable := result.Stable
 	var stables []string
 	for _, sta := range result.Stables {
@@ -736,11 +735,11 @@ func (rm *resmon) RegisterResource(ctx context.Context,
 	}
 	logging.V(5).Infof(
 		"ResourceMonitor.RegisterResource operation finished: t=%v, urn=%v, stable=%v, #stables=%v #outs=%v",
-		state.Type, state.URN, stable, len(stables), len(props))
+		state.Type, state.URN, stable, len(stables), len(state.Outputs))
 
 	// Finally, unpack the response into properties that we can return to the language runtime.  This mostly includes
 	// an ID, URN, and defaults and output properties that will all be blitted back onto the runtime object.
-	obj, err := plugin.MarshalProperties(props, plugin.MarshalOptions{Label: label, KeepUnknowns: true})
+	obj, err := plugin.MarshalProperties(state.Outputs, plugin.MarshalOptions{Label: label, KeepUnknowns: true})
 	if err != nil {
 		return nil, err
 	}

pkg/resource/deploy/step.go

@@ -302,7 +302,7 @@ func (s *DeleteStep) Apply(preview bool) (resource.Status, StepCompleteFunc, err
 			if err != nil {
 				return resource.StatusOK, nil, err
 			}
-			if rst, err := prov.Delete(s.URN(), s.old.ID, s.old.All()); err != nil {
+			if rst, err := prov.Delete(s.URN(), s.old.ID, s.old.Outputs); err != nil {
 				return rst, nil, err
 			}
 		}
@@ -403,8 +403,8 @@ func (s *UpdateStep) Apply(preview bool) (resource.Status, StepCompleteFunc, err
 				return resource.StatusOK, nil, err
 			}
 
-			// Update to the combination of the old "all" state (including outputs), but overwritten with new inputs.
-			outs, rst, upderr := prov.Update(s.URN(), s.old.ID, s.old.All(), s.new.Inputs)
+			// Update to the combination of the old "all" state, but overwritten with new inputs.
+			outs, rst, upderr := prov.Update(s.URN(), s.old.ID, s.old.Outputs, s.new.Inputs)
 			if upderr != nil {
 				if rst != resource.StatusPartialFailure {
 					return rst, nil, upderr

pkg/resource/properties.go

@@ -148,20 +148,6 @@ func (m PropertyMap) Copy() PropertyMap {
 	return new
 }
 
-// Merge simply merges in another map atop another, and returns the result.
-func (m PropertyMap) Merge(other PropertyMap) PropertyMap {
-	new := m.Copy()
-	for k, v := range other {
-		if !v.IsNull() {
-			if mv, ok := m[k]; ok {
-				v = mv.merge(v)
-			}
-			new[k] = v
-		}
-	}
-	return new
-}
-
 // StableKeys returns all of the map's keys in a stable order.
 func (m PropertyMap) StableKeys() []PropertyKey {
 	sorted := make([]PropertyKey, 0, len(m))
@@ -471,42 +457,6 @@ func (v PropertyValue) MapRepl(replk func(string) (string, bool),
 	return v.ObjectValue().MapRepl(replk, replv)
 }
 
-// merge simply merges the value of other into v. Merging proceeds as follows:
-// - If other is null, v is returned.
-// - If v and other are both arrays, the corresponding elements are recurively merged. Any unmerged elements in v or
-//   other are then appended to the result.
-// - If v and other are both maps, the corresponding key-value pairs are recursively merged.
-// - Otherwise, other is returned.
-func (v PropertyValue) merge(other PropertyValue) PropertyValue {
-	switch {
-	case other.IsNull():
-		return v
-	case v.IsArray() && other.IsArray():
-		left, right, merged := v.ArrayValue(), other.ArrayValue(), []PropertyValue{}
-		for len(left) > 0 && len(right) > 0 {
-			merged = append(merged, left[0].merge(right[0]))
-			left, right = left[1:], right[1:]
-		}
-		switch {
-		case len(left) > 0:
-			contract.Assert(len(right) == 0)
-			for ; len(left) > 0; left = left[1:] {
-				merged = append(merged, left[0])
-			}
-		case len(right) > 0:
-			contract.Assert(len(left) == 0)
-			for ; len(right) > 0; right = right[1:] {
-				merged = append(merged, right[0])
-			}
-		}
-		return NewArrayProperty(merged)
-	case v.IsObject() && other.IsObject():
-		return NewObjectProperty(v.ObjectValue().Merge(other.ObjectValue()))
-	default:
-		return other
-	}
-}
-
 // String implements the fmt.Stringer interface to add slightly more information to the output.
 func (v PropertyValue) String() string {
 	if v.IsComputed() || v.IsOutput() {

pkg/resource/resource_state.go

@@ -68,8 +68,3 @@ func NewState(t tokens.Type, urn URN, custom bool, del bool, id ID,
 		PendingReplacement:   pendingReplacement,
 	}
 }
-
-// All returns all resource state, including the inputs and outputs, overlaid in that order.
-func (s *State) All() PropertyMap {
-	return s.Inputs.Merge(s.Outputs)
-}