Allow resource IDs to change on reresh steps (#3087)

* Allow resource IDs to change on reresh steps

This is a requirement for us to be able to move forward with
versions of the Terraform Azurerm provider. In v1.32.1, there was
a state migration that changed the ID format of the azure table
storage resource

We used to have a check in place for old ID being equal to new ID.
This has been changed now and we allow the change of ID to happen
in the RefreshStep

* Update pkg/resource/deploy/step.go

Co-Authored-By: Pat Gavlin <pat@pulumi.com>

CHANGELOG.md

@@ -6,6 +6,8 @@ CHANGELOG
 - When using StackReference to fetch output values from another stack, do not mark a value as secret if it was not
   secret in the stack you referenced. (fixes [#2744](https://github.com/pulumi/pulumi/issues/2744)).
 
+- Allow resource IDs to be changed during `pulumi refresh` operations
+
 ## 1.0.0-beta.2 (2019-08-13)
 
 - Fix the package version compatibility checks in the NodeJS language host.

pkg/engine/lifecycle_test.go

@@ -23,8 +23,6 @@ import (
 	"sync"
 	"testing"
 
-	"github.com/pulumi/pulumi/pkg/secrets"
-
 	"github.com/blang/semver"
 	"github.com/mitchellh/copystructure"
 	"github.com/pkg/errors"
@@ -39,6 +37,7 @@ import (
 	"github.com/pulumi/pulumi/pkg/resource/deploy/deploytest"
 	"github.com/pulumi/pulumi/pkg/resource/deploy/providers"
 	"github.com/pulumi/pulumi/pkg/resource/plugin"
+	"github.com/pulumi/pulumi/pkg/secrets"
 	"github.com/pulumi/pulumi/pkg/tokens"
 	"github.com/pulumi/pulumi/pkg/util/cancel"
 	"github.com/pulumi/pulumi/pkg/util/contract"
@@ -4035,3 +4034,63 @@ func TestProviderDiffMissingOldOutputs(t *testing.T) {
 	}}
 	p.Run(t, snap)
 }
+
+func TestRefreshStepWillPersistUpdatedIDs(t *testing.T) {
+	p := &TestPlan{}
+
+	provURN := p.NewProviderURN("pkgA", "default", "")
+	resURN := p.NewURN("pkgA:m:typA", "resA", "")
+	idBefore := resource.ID("myid")
+	idAfter := resource.ID("mynewid")
+	outputs := resource.PropertyMap{"foo": resource.NewStringProperty("bar")}
+
+	loaders := []*deploytest.ProviderLoader{
+		deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) {
+			return &deploytest.Provider{
+				ReadF: func(
+					urn resource.URN, id resource.ID, inputs, state resource.PropertyMap,
+				) (plugin.ReadResult, resource.Status, error) {
+					return plugin.ReadResult{ID: idAfter, Outputs: outputs, Inputs: resource.PropertyMap{}}, resource.StatusOK, nil
+				},
+			}, nil
+		}),
+	}
+
+	program := deploytest.NewLanguageRuntime(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
+		_, _, _, err := monitor.RegisterResource("pkgA:m:typA", "resA", false)
+		assert.NoError(t, err)
+		return nil
+	})
+	host := deploytest.NewPluginHost(nil, nil, program, loaders...)
+
+	p.Options.host = host
+
+	old := &deploy.Snapshot{
+		Resources: []*resource.State{
+			{
+				Type:       resURN.Type(),
+				URN:        resURN,
+				Custom:     true,
+				ID:         idBefore,
+				Inputs:     resource.PropertyMap{},
+				Outputs:    outputs,
+				InitErrors: []string{"Resource failed to initialize"},
+			},
+		},
+	}
+
+	p.Steps = []TestStep{{Op: Refresh, SkipPreview: true}}
+	snap := p.Run(t, old)
+
+	for _, resource := range snap.Resources {
+		switch urn := resource.URN; urn {
+		case provURN:
+			// break
+		case resURN:
+			assert.Empty(t, resource.InitErrors)
+			assert.Equal(t, idAfter, resource.ID)
+		default:
+			t.Fatalf("unexpected resource %v", urn)
+		}
+	}
+}

pkg/resource/deploy/step.go

@@ -27,6 +27,7 @@ import (
 	"github.com/pulumi/pulumi/pkg/resource/plugin"
 	"github.com/pulumi/pulumi/pkg/tokens"
 	"github.com/pulumi/pulumi/pkg/util/contract"
+	"github.com/pulumi/pulumi/pkg/util/logging"
 )
 
 // StepCompleteFunc is the type of functions returned from Step.Apply. These functions are to be called
@@ -666,6 +667,8 @@ func (s *RefreshStep) Apply(preview bool) (resource.Status, StepCompleteFunc, er
 		complete = func() { close(s.done) }
 	}
 
+	resourceID := s.old.ID
+
 	// Component, provider, and pending-replace resources never change with a refresh; just return the current state.
 	if !s.old.Custom || providers.IsProviderType(s.old.Type) || s.old.PendingReplacement {
 		return resource.StatusOK, complete, nil
@@ -678,7 +681,7 @@ func (s *RefreshStep) Apply(preview bool) (resource.Status, StepCompleteFunc, er
 	}
 
 	var initErrors []string
-	refreshed, rst, err := prov.Read(s.old.URN, s.old.ID, s.old.Inputs, s.old.Outputs)
+	refreshed, rst, err := prov.Read(s.old.URN, resourceID, s.old.Inputs, s.old.Outputs)
 	if err != nil {
 		if rst != resource.StatusPartialFailure {
 			return rst, nil, err
@@ -705,7 +708,15 @@ func (s *RefreshStep) Apply(preview bool) (resource.Status, StepCompleteFunc, er
 	}
 
 	if outputs != nil {
-		s.new = resource.NewState(s.old.Type, s.old.URN, s.old.Custom, s.old.Delete, s.old.ID, inputs, outputs,
+		// There is a chance that the ID has changed. We want to allow this change to happen
+		// it will have changed already in the outputs, but we need to persist this change
+		// at a state level because the Id
+		if refreshed.ID != "" && refreshed.ID != resourceID {
+			logging.V(7).Infof("Refreshing ID; oldId=%s, newId=%s", resourceID, refreshed.ID)
+			resourceID = refreshed.ID
+		}
+
+		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)

pkg/resource/plugin/provider.go

@@ -190,6 +190,9 @@ func (e DiffUnavailableError) Error() string {
 
 // ReadResult is the result of a call to Read.
 type ReadResult struct {
+	// This is the ID for the resource. This ID will always be populated and will ensure we get the most up-to-date
+	// resource ID.
+	ID resource.ID
 	// Inputs contains the new inputs for the resource, if any. If this field is nil, the provider does not support
 	// returning inputs from a call to Read and the old inputs (if any) should be preserved.
 	Inputs resource.PropertyMap

pkg/resource/plugin/provider_plugin.go

@@ -734,9 +734,6 @@ func (p *provider) Read(urn resource.URN, id resource.ID,
 	// If the resource was missing, simply return a nil property map.
 	if string(readID) == "" {
 		return ReadResult{}, resourceStatus, nil
-	} else if readID != id {
-		return ReadResult{}, resourceStatus, errors.Errorf(
-			"reading resource %s yielded an unexpected ID; expected %s, got %s", urn, id, readID)
 	}
 
 	// Finally, unmarshal the resulting state properties and return them.
@@ -771,6 +768,7 @@ func (p *provider) Read(urn resource.URN, id resource.ID,
 
 	logging.V(7).Infof("%s success; #outs=%d, #inputs=%d", label, len(newState), len(newInputs))
 	return ReadResult{
+		ID:      readID,
 		Outputs: newState,
 		Inputs:  newInputs,
 	}, resourceStatus, resourceError