Avoid replace on second update with import applied (#4403)

After importing some resources, and running a second update with the
import still applied, an unexpected replace would occur. This wouldn't
happen for the vast majority of resources, but for some it would.

It turns out that the resources that trigger this are ones that use a
different format of identifier for the import input than they do for the
ID property.

Before this change, we would trigger an import-replacement when an
existing resource's ID property didn't match the import property, which
would be the case for the small set of resources where the input
identifier is different than the ID property.

To avoid this, we now store the `importID` in the statefile, and
compare that to the import property instead of comparing the ID.

CHANGELOG.md

@@ -34,6 +34,8 @@ CHANGELOG
 - Switch .NET projects to .NET Core 3.1
   [#4400](https://github.com/pulumi/pulumi/pull/4400)
 
+- Avoid unexpected replace on resource with `import` applied on second update.
+  [#4403](https://github.com/pulumi/pulumi/pull/4403)
 
 ## 1.14.1 (2020-04-13)
 - Propagate `additionalSecretOutputs` opt to Read in NodeJS.

pkg/backend/display/json.go

@@ -85,7 +85,8 @@ func stateForJSONOutput(s *resource.State, opts Options) *resource.State {
 
 	return resource.NewState(s.Type, s.URN, s.Custom, s.Delete, s.ID, inputs,
 		outputs, s.Parent, s.Protect, s.External, s.Dependencies, s.InitErrors, s.Provider,
-		s.PropertyDependencies, s.PendingReplacement, s.AdditionalSecretOutputs, s.Aliases, &s.CustomTimeouts)
+		s.PropertyDependencies, s.PendingReplacement, s.AdditionalSecretOutputs, s.Aliases, &s.CustomTimeouts,
+		s.ImportID)
 }
 
 // ShowJSONEvents renders engine events from a preview into a well-formed JSON document. Note that this does not

pkg/engine/lifecycle_test.go

@@ -4144,6 +4144,103 @@ func TestImport(t *testing.T) {
 	assert.Nil(t, res)
 }
 
+// TestImportWithDifferingImportIdentifierFormat tests importing a resource that has a different format of identifier
+// for the import input than for the ID property, ensuring that a second update does not result in a replace.
+func TestImportWithDifferingImportIdentifierFormat(t *testing.T) {
+	loaders := []*deploytest.ProviderLoader{
+		deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) {
+			return &deploytest.Provider{
+				DiffF: func(urn resource.URN, id resource.ID,
+					olds, news resource.PropertyMap, ignoreChanges []string) (plugin.DiffResult, error) {
+
+					if olds["foo"].DeepEquals(news["foo"]) {
+						return plugin.DiffResult{Changes: plugin.DiffNone}, nil
+					}
+
+					return plugin.DiffResult{
+						Changes: plugin.DiffSome,
+						DetailedDiff: map[string]plugin.PropertyDiff{
+							"foo": {Kind: plugin.DiffUpdate},
+						},
+					}, nil
+				},
+				CreateF: func(urn resource.URN,
+					news resource.PropertyMap, timeout float64) (resource.ID, resource.PropertyMap, resource.Status, error) {
+
+					return "created-id", news, resource.StatusOK, nil
+				},
+				ReadF: func(urn resource.URN, id resource.ID,
+					inputs, state resource.PropertyMap) (plugin.ReadResult, resource.Status, error) {
+
+					return plugin.ReadResult{
+						// This ID is deliberately not the same as the ID used to import.
+						ID: "id",
+						Inputs: resource.PropertyMap{
+							"foo": resource.NewStringProperty("bar"),
+						},
+						Outputs: resource.PropertyMap{
+							"foo": resource.NewStringProperty("bar"),
+						},
+					}, resource.StatusOK, nil
+				},
+			}, nil
+		}),
+	}
+
+	program := deploytest.NewLanguageRuntime(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
+		_, _, _, err := monitor.RegisterResource("pkgA:m:typA", "resA", true, deploytest.ResourceOptions{
+			Inputs: resource.PropertyMap{
+				"foo": resource.NewStringProperty("bar"),
+			},
+			// The import ID is deliberately not the same as the ID returned from Read.
+			ImportID: resource.ID("import-id"),
+		})
+		assert.NoError(t, err)
+		return nil
+	})
+	host := deploytest.NewPluginHost(nil, nil, program, loaders...)
+
+	p := &TestPlan{
+		Options: UpdateOptions{host: host},
+	}
+	provURN := p.NewProviderURN("pkgA", "default", "")
+	resURN := p.NewURN("pkgA:m:typA", "resA", "")
+
+	// Run the initial update. The import should succeed.
+	project := p.GetProject()
+	snap, res := TestOp(Update).Run(project, p.GetTarget(nil), p.Options, false, p.BackendClient,
+		func(_ workspace.Project, _ deploy.Target, j *Journal, _ []Event, res result.Result) result.Result {
+			for _, entry := range j.Entries {
+				switch urn := entry.Step.URN(); urn {
+				case provURN:
+					assert.Equal(t, deploy.OpCreate, entry.Step.Op())
+				case resURN:
+					assert.Equal(t, deploy.OpImport, entry.Step.Op())
+				default:
+					t.Fatalf("unexpected resource %v", urn)
+				}
+			}
+			return res
+		})
+	assert.Nil(t, res)
+	assert.Len(t, snap.Resources, 2)
+
+	// Now, run another update. The update should succeed and there should be no diffs.
+	snap, res = TestOp(Update).Run(project, p.GetTarget(snap), p.Options, false, p.BackendClient,
+		func(_ workspace.Project, _ deploy.Target, j *Journal, _ []Event, res result.Result) result.Result {
+			for _, entry := range j.Entries {
+				switch urn := entry.Step.URN(); urn {
+				case provURN, resURN:
+					assert.Equal(t, deploy.OpSame, entry.Step.Op())
+				default:
+					t.Fatalf("unexpected resource %v", urn)
+				}
+			}
+			return res
+		})
+	assert.Nil(t, res)
+}
+
 func TestCustomTimeouts(t *testing.T) {
 	loaders := []*deploytest.ProviderLoader{
 		deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) {

pkg/resource/deploy/source_eval_test.go

@@ -67,7 +67,7 @@ func fixedProgram(steps []RegisterResourceEvent) deploytest.ProgramFunc {
 			}
 			s.Done(&RegisterResult{
 				State: resource.NewState(g.Type, urn, g.Custom, false, id, g.Properties, outs, g.Parent, g.Protect,
-					false, g.Dependencies, nil, g.Provider, g.PropertyDependencies, false, nil, nil, nil),
+					false, g.Dependencies, nil, g.Provider, g.PropertyDependencies, false, nil, nil, nil, ""),
 			})
 		}
 		return nil
@@ -204,7 +204,7 @@ func TestRegisterNoDefaultProviders(t *testing.T) {
 		reg.Done(&RegisterResult{
 			State: resource.NewState(goal.Type, urn, goal.Custom, false, id, goal.Properties, resource.PropertyMap{},
 				goal.Parent, goal.Protect, false, goal.Dependencies, nil, goal.Provider, goal.PropertyDependencies,
-				false, nil, nil, nil),
+				false, nil, nil, nil, ""),
 		})
 
 		processed++
@@ -296,7 +296,7 @@ func TestRegisterDefaultProviders(t *testing.T) {
 		reg.Done(&RegisterResult{
 			State: resource.NewState(goal.Type, urn, goal.Custom, false, id, goal.Properties, resource.PropertyMap{},
 				goal.Parent, goal.Protect, false, goal.Dependencies, nil, goal.Provider, goal.PropertyDependencies,
-				false, nil, nil, nil),
+				false, nil, nil, nil, ""),
 		})
 
 		processed++
@@ -386,7 +386,7 @@ func TestReadInvokeNoDefaultProviders(t *testing.T) {
 		read.Done(&ReadResult{
 			State: resource.NewState(read.Type(), urn, true, false, read.ID(), read.Properties(),
 				resource.PropertyMap{}, read.Parent(), false, false, read.Dependencies(), nil, read.Provider(), nil,
-				false, nil, nil, nil),
+				false, nil, nil, nil, ""),
 		})
 		reads++
 	}
@@ -471,7 +471,7 @@ func TestReadInvokeDefaultProviders(t *testing.T) {
 			e.Done(&RegisterResult{
 				State: resource.NewState(goal.Type, urn, goal.Custom, false, id, goal.Properties, resource.PropertyMap{},
 					goal.Parent, goal.Protect, false, goal.Dependencies, nil, goal.Provider, goal.PropertyDependencies,
-					false, nil, nil, nil),
+					false, nil, nil, nil, ""),
 			})
 			registers++
 
@@ -480,7 +480,7 @@ func TestReadInvokeDefaultProviders(t *testing.T) {
 			e.Done(&ReadResult{
 				State: resource.NewState(e.Type(), urn, true, false, e.ID(), e.Properties(),
 					resource.PropertyMap{}, e.Parent(), false, false, e.Dependencies(), nil, e.Provider(), nil, false,
-					nil, nil, nil),
+					nil, nil, nil, ""),
 			})
 			reads++
 		}

pkg/resource/deploy/step.go

@@ -761,7 +761,7 @@ func (s *RefreshStep) Apply(preview bool) (resource.Status, StepCompleteFunc, er
 		s.new = resource.NewState(s.old.Type, s.old.URN, s.old.Custom, s.old.Delete, resourceID, inputs, outputs,
 			s.old.Parent, s.old.Protect, s.old.External, s.old.Dependencies, initErrors, s.old.Provider,
 			s.old.PropertyDependencies, s.old.PendingReplacement, s.old.AdditionalSecretOutputs, s.old.Aliases,
-			&s.old.CustomTimeouts)
+			&s.old.CustomTimeouts, s.old.ImportID)
 	} else {
 		s.new = nil
 	}
@@ -871,7 +871,7 @@ func (s *ImportStep) Apply(preview bool) (resource.Status, StepCompleteFunc, err
 	// differences between the old and new states are between the inputs and outputs.
 	s.old = resource.NewState(s.new.Type, s.new.URN, s.new.Custom, false, s.new.ID, read.Inputs, read.Outputs,
 		s.new.Parent, s.new.Protect, false, s.new.Dependencies, s.new.InitErrors, s.new.Provider,
-		s.new.PropertyDependencies, false, nil, nil, &s.new.CustomTimeouts)
+		s.new.PropertyDependencies, false, nil, nil, &s.new.CustomTimeouts, s.new.ImportID)
 
 	// Check the user inputs using the provider inputs for defaults.
 	inputs, failures, err := prov.Check(s.new.URN, s.old.Inputs, s.new.Inputs, preview)

pkg/resource/deploy/step_generator.go

@@ -109,6 +109,7 @@ func (sg *stepGenerator) GenerateReadSteps(event ReadResourceEvent) ([]Step, res
 		event.AdditionalSecretOutputs(),
 		nil, /* aliases */
 		nil, /* customTimeouts */
+		"",  /* importID */
 	)
 	old, hasOld := sg.plan.Olds()[urn]
 
@@ -250,7 +251,7 @@ func (sg *stepGenerator) generateSteps(event RegisterResourceEvent) ([]Step, res
 	// get serialized into the checkpoint file.
 	new := resource.NewState(goal.Type, urn, goal.Custom, false, "", inputs, nil, goal.Parent, goal.Protect, false,
 		goal.Dependencies, goal.InitErrors, goal.Provider, goal.PropertyDependencies, false,
-		goal.AdditionalSecretOutputs, goal.Aliases, &goal.CustomTimeouts)
+		goal.AdditionalSecretOutputs, goal.Aliases, &goal.CustomTimeouts, "")
 
 	// Mark the URN/resource as having been seen. So we can run analyzers on all resources seen, as well as
 	// lookup providers for calculating replacement of resources that use the provider.
@@ -277,11 +278,21 @@ func (sg *stepGenerator) generateSteps(event RegisterResourceEvent) ([]Step, res
 
 	// If the goal contains an ID, this may be an import. An import occurs if there is no old resource or if the old
 	// resource's ID does not match the ID in the goal state.
-	isImport := goal.Custom && goal.ID != "" && (!hasOld || old.External || old.ID != goal.ID)
+	var oldImportID resource.ID
+	if hasOld {
+		oldImportID = old.ID
+		// If the old resource has an ImportID, look at that rather than the ID, since some resources use a different
+		// format of identifier for the import input than the ID property.
+		if old.ImportID != "" {
+			oldImportID = old.ImportID
+		}
+	}
+	isImport := goal.Custom && goal.ID != "" && (!hasOld || old.External || oldImportID != goal.ID)
 	if isImport {
 		// Write the ID of the resource to import into the new state and return an ImportStep or an
 		// ImportReplacementStep
 		new.ID = goal.ID
+		new.ImportID = goal.ID
 		if isReplace := hasOld && !recreating; isReplace {
 			return []Step{
 				NewImportReplacementStep(sg.plan, event, old, new, goal.IgnoreChanges),

pkg/resource/stack/deployment.go

@@ -286,6 +286,7 @@ func SerializeResource(res *resource.State, enc config.Encrypter) (apitype.Resou
 		PendingReplacement:      res.PendingReplacement,
 		AdditionalSecretOutputs: res.AdditionalSecretOutputs,
 		Aliases:                 res.Aliases,
+		ImportID:                res.ImportID,
 	}
 
 	if res.CustomTimeouts.IsNotEmpty() {
@@ -410,7 +411,8 @@ func DeserializeResource(res apitype.ResourceV3, dec config.Decrypter) (*resourc
 	return resource.NewState(
 		res.Type, res.URN, res.Custom, res.Delete, res.ID,
 		inputs, outputs, res.Parent, res.Protect, res.External, res.Dependencies, res.InitErrors, res.Provider,
-		res.PropertyDependencies, res.PendingReplacement, res.AdditionalSecretOutputs, res.Aliases, res.CustomTimeouts), nil
+		res.PropertyDependencies, res.PendingReplacement, res.AdditionalSecretOutputs, res.Aliases, res.CustomTimeouts,
+		res.ImportID), nil
 }
 
 func DeserializeOperation(op apitype.OperationV2, dec config.Decrypter) (resource.Operation, error) {

pkg/resource/stack/deployment_test.go

@@ -84,6 +84,7 @@ func TestDeploymentSerialization(t *testing.T) {
 		nil,
 		nil,
 		nil,
+		"",
 	)
 
 	dep, err := SerializeResource(res, config.NopEncrypter)

sdk/go/common/apitype/core.go

@@ -287,6 +287,8 @@ type ResourceV3 struct {
 	Aliases []resource.URN `json:"aliases,omitempty" yaml:"aliases,omitempty"`
 	// CustomTimeouts is a configuration block that can be used to control timeouts of CRUD operations
 	CustomTimeouts *resource.CustomTimeouts `json:"customTimeouts,omitempty" yaml:"customTimeouts,omitempty"`
+	// ImportID is the import input used for imported resources.
+	ImportID resource.ID `json:"importID,omitempty" yaml:"importID,omitempty"`
 }
 
 // ManifestV1 captures meta-information about this checkpoint file, such as versions of binaries, etc.

sdk/go/common/resource/resource_state.go

@@ -42,6 +42,7 @@ type State struct {
 	AdditionalSecretOutputs []PropertyKey         // an additional set of outputs that should be treated as secrets.
 	Aliases                 []URN                 // TODO
 	CustomTimeouts          CustomTimeouts        // A config block that will be used to configure timeouts for CRUD operations
+	ImportID                ID                    // the resource's import id, if this was an imported resource.
 }
 
 // NewState creates a new resource value from existing resource state information.
@@ -49,7 +50,8 @@ func NewState(t tokens.Type, urn URN, custom bool, del bool, id ID,
 	inputs PropertyMap, outputs PropertyMap, parent URN, protect bool,
 	external bool, dependencies []URN, initErrors []string, provider string,
 	propertyDependencies map[PropertyKey][]URN, pendingReplacement bool,
-	additionalSecretOutputs []PropertyKey, aliases []URN, timeouts *CustomTimeouts) *State {
+	additionalSecretOutputs []PropertyKey, aliases []URN, timeouts *CustomTimeouts,
+	importID ID) *State {
 
 	contract.Assertf(t != "", "type was empty")
 	contract.Assertf(custom || id == "", "is custom or had empty ID")
@@ -73,6 +75,7 @@ func NewState(t tokens.Type, urn URN, custom bool, del bool, id ID,
 		PendingReplacement:      pendingReplacement,
 		AdditionalSecretOutputs: additionalSecretOutputs,
 		Aliases:                 aliases,
+		ImportID:                importID,
 	}
 
 	if timeouts != nil {