Merge pull request #21611 from hashicorp/jbardin/private-data-read

Make sure resource private data is carried through the entire resource lifecycle

builtin/providers/test/resource_timeout_test.go

@@ -27,6 +27,27 @@ resource "test_resource_timeout" "foo" {
 		},
 	})
 }
+
+func TestResourceTimeout_delete(t *testing.T) {
+	// If the delete timeout isn't saved until destroy, the cleanup here will
+	// fail because the default is only 20m.
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_timeout" "foo" {
+	delete_delay = "25m"
+	timeouts {
+		delete = "30m"
+	}
+}
+				`),
+			},
+		},
+	})
+}
 func TestResourceTimeout_update(t *testing.T) {
 	resource.UnitTest(t, resource.TestCase{
 		Providers:    testAccProviders,

command/apply_test.go

@@ -15,6 +15,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/mitchellh/cli"
 	"github.com/zclconf/go-cty/cty"
 
@@ -1392,7 +1393,7 @@ func TestApply_backup(t *testing.T) {
 
 	actual := backupState.RootModule().Resources["test_instance.foo"]
 	expected := originalState.RootModule().Resources["test_instance.foo"]
-	if !cmp.Equal(actual, expected) {
+	if !cmp.Equal(actual, expected, cmpopts.EquateEmpty()) {
 		t.Fatalf(
 			"wrong aws_instance.foo state\n%s",
 			cmp.Diff(expected, actual, cmp.Transformer("bytesAsString", func(b []byte) string {

command/command_test.go

@@ -7,8 +7,6 @@ import (
 	"encoding/json"
 	"flag"
 	"fmt"
-	"github.com/hashicorp/terraform/internal/initwd"
-	"github.com/hashicorp/terraform/registry"
 	"io"
 	"io/ioutil"
 	"log"
@@ -21,6 +19,9 @@ import (
 	"syscall"
 	"testing"
 
+	"github.com/hashicorp/terraform/internal/initwd"
+	"github.com/hashicorp/terraform/registry"
+
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/configs/configload"
@@ -266,7 +267,10 @@ func testState() *states.State {
 				Type: "test",
 			}.Absolute(addrs.RootModuleInstance),
 		)
-	})
+		// DeepCopy is used here to ensure our synthetic state matches exactly
+		// with a state that will have been copied during the command
+		// operation, and all fields have been copied correctly.
+	}).DeepCopy()
 }
 
 // writeStateForTesting is a helper that writes the given naked state to the

command/refresh_test.go

@@ -12,6 +12,8 @@ import (
 	"testing"
 
 	"github.com/davecgh/go-spew/spew"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/mitchellh/cli"
 	"github.com/zclconf/go-cty/cty"
 
@@ -557,8 +559,8 @@ func TestRefresh_backup(t *testing.T) {
 	}
 
 	newState := testStateRead(t, statePath)
-	if !reflect.DeepEqual(newState, state) {
-		t.Fatalf("bad: %#v", newState)
+	if !cmp.Equal(newState, state, cmpopts.EquateEmpty()) {
+		t.Fatalf("got:\n%s\nexpected:\n%s\n", newState, state)
 	}
 
 	newState = testStateRead(t, outPath)

helper/plugin/grpc_provider.go

@@ -535,6 +535,11 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso
 		Msgpack: newStateMP,
 	}
 
+	// helper/schema did previously handle private data during refresh, but
+	// core is now going to expect this to be maintained in order to
+	// persist it in the state.
+	resp.Private = req.Private
+
 	return resp, nil
 }
 
@@ -569,6 +574,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl
 	// We don't usually plan destroys, but this can return early in any case.
 	if proposedNewStateVal.IsNull() {
 		resp.PlannedState = req.ProposedNewState
+		resp.PlannedPrivate = req.PriorPrivate
 		return resp, nil
 	}
 

helper/resource/state_shim.go

@@ -1,6 +1,7 @@
 package resource
 
 import (
+	"encoding/json"
 	"fmt"
 
 	"github.com/hashicorp/terraform/addrs"
@@ -52,43 +53,57 @@ func shimNewState(newState *states.State, providers map[string]terraform.Resourc
 			resource := getResource(providers, providerType, res.Addr)
 
 			for key, i := range res.Instances {
-				flatmap, err := shimmedAttributes(i.Current, resource)
-				if err != nil {
-					return nil, fmt.Errorf("error decoding state for %q: %s", resType, err)
+				resState := &terraform.ResourceState{
+					Type:     resType,
+					Provider: res.ProviderConfig.String(),
 				}
 
-				resState := &terraform.ResourceState{
-					Type: resType,
-					Primary: &terraform.InstanceState{
+				// We should always have a Current instance here, but be safe about checking.
+				if i.Current != nil {
+					flatmap, err := shimmedAttributes(i.Current, resource)
+					if err != nil {
+						return nil, fmt.Errorf("error decoding state for %q: %s", resType, err)
+					}
+
+					var meta map[string]interface{}
+					if i.Current.Private != nil {
+						err := json.Unmarshal(i.Current.Private, &meta)
+						if err != nil {
+							return nil, err
+						}
+					}
+
+					resState.Primary = &terraform.InstanceState{
 						ID:         flatmap["id"],
 						Attributes: flatmap,
 						Tainted:    i.Current.Status == states.ObjectTainted,
-					},
-					Provider: res.ProviderConfig.String(),
-				}
-				if i.Current.SchemaVersion != 0 {
-					resState.Primary.Meta = map[string]interface{}{
-						"schema_version": i.Current.SchemaVersion,
+						Meta:       meta,
 					}
-				}
 
-				for _, dep := range i.Current.Dependencies {
-					resState.Dependencies = append(resState.Dependencies, dep.String())
-				}
-
-				// convert the indexes to the old style flapmap indexes
-				idx := ""
-				switch key.(type) {
-				case addrs.IntKey:
-					// don't add numeric index values to resources with a count of 0
-					if len(res.Instances) > 1 {
-						idx = fmt.Sprintf(".%d", key)
+					if i.Current.SchemaVersion != 0 {
+						resState.Primary.Meta = map[string]interface{}{
+							"schema_version": i.Current.SchemaVersion,
+						}
 					}
-				case addrs.StringKey:
-					idx = "." + key.String()
-				}
 
-				mod.Resources[res.Addr.String()+idx] = resState
+					for _, dep := range i.Current.Dependencies {
+						resState.Dependencies = append(resState.Dependencies, dep.String())
+					}
+
+					// convert the indexes to the old style flapmap indexes
+					idx := ""
+					switch key.(type) {
+					case addrs.IntKey:
+						// don't add numeric index values to resources with a count of 0
+						if len(res.Instances) > 1 {
+							idx = fmt.Sprintf(".%d", key)
+						}
+					case addrs.StringKey:
+						idx = "." + key.String()
+					}
+
+					mod.Resources[res.Addr.String()+idx] = resState
+				}
 
 				// add any deposed instances
 				for _, dep := range i.Deposed {
@@ -97,10 +112,19 @@ func shimNewState(newState *states.State, providers map[string]terraform.Resourc
 						return nil, fmt.Errorf("error decoding deposed state for %q: %s", resType, err)
 					}
 
+					var meta map[string]interface{}
+					if dep.Private != nil {
+						err := json.Unmarshal(dep.Private, &meta)
+						if err != nil {
+							return nil, err
+						}
+					}
+
 					deposed := &terraform.InstanceState{
 						ID:         flatmap["id"],
 						Attributes: flatmap,
 						Tainted:    dep.Status == states.ObjectTainted,
+						Meta:       meta,
 					}
 					if dep.SchemaVersion != 0 {
 						deposed.Meta = map[string]interface{}{

internal/tfplugin5/tfplugin5.proto

@@ -219,10 +219,12 @@ message ReadResource {
     message Request {
         string type_name = 1;
         DynamicValue current_state = 2;
+        bytes private = 3;
     }
     message Response {
         DynamicValue new_state = 1;
         repeated Diagnostic diagnostics = 2;
+        bytes private = 3;
     }
 }
 

plugin/grpc_provider.go

@@ -330,6 +330,7 @@ func (p *GRPCProvider) ReadResource(r providers.ReadResourceRequest) (resp provi
 	protoReq := &proto.ReadResource_Request{
 		TypeName:     r.TypeName,
 		CurrentState: &proto.DynamicValue{Msgpack: mp},
+		Private:      r.Private,
 	}
 
 	protoResp, err := p.client.ReadResource(p.ctx, protoReq)
@@ -348,6 +349,7 @@ func (p *GRPCProvider) ReadResource(r providers.ReadResourceRequest) (resp provi
 		}
 	}
 	resp.NewState = state
+	resp.Private = protoResp.Private
 
 	return resp
 }

providers/provider.go

@@ -176,6 +176,10 @@ type ReadResourceRequest struct {
 
 	// PriorState contains the previously saved state value for this resource.
 	PriorState cty.Value
+
+	// Private is an opaque blob that will be stored in state along with the
+	// resource. It is intended only for interpretation by the provider itself.
+	Private []byte
 }
 
 type ReadResourceResponse struct {
@@ -184,6 +188,10 @@ type ReadResourceResponse struct {
 
 	// Diagnostics contains any warnings or errors from the method call.
 	Diagnostics tfdiags.Diagnostics
+
+	// Private is an opaque blob that will be stored in state along with the
+	// resource. It is intended only for interpretation by the provider itself.
+	Private []byte
 }
 
 type PlanResourceChangeRequest struct {

states/state_deepcopy.go

@@ -147,7 +147,7 @@ func (obj *ResourceInstanceObjectSrc) DeepCopy() *ResourceInstanceObjectSrc {
 
 	var private []byte
 	if obj.Private != nil {
-		private := make([]byte, len(obj.Private))
+		private = make([]byte, len(obj.Private))
 		copy(private, obj.Private)
 	}
 
@@ -181,14 +181,17 @@ func (obj *ResourceInstanceObject) DeepCopy() *ResourceInstanceObject {
 
 	var private []byte
 	if obj.Private != nil {
-		private := make([]byte, len(obj.Private))
+		private = make([]byte, len(obj.Private))
 		copy(private, obj.Private)
 	}
 
-	// Some addrs.Referencable implementations are technically mutable, but
+	// Some addrs.Referenceable implementations are technically mutable, but
 	// we treat them as immutable by convention and so we don't deep-copy here.
-	dependencies := make([]addrs.Referenceable, len(obj.Dependencies))
-	copy(dependencies, obj.Dependencies)
+	var dependencies []addrs.Referenceable
+	if obj.Dependencies != nil {
+		dependencies = make([]addrs.Referenceable, len(obj.Dependencies))
+		copy(dependencies, obj.Dependencies)
+	}
 
 	return &ResourceInstanceObject{
 		Value:        obj.Value,

states/state_test.go

@@ -115,3 +115,62 @@ func TestState(t *testing.T) {
 		t.Error(problem)
 	}
 }
+
+func TestStateDeepCopy(t *testing.T) {
+	state := NewState()
+
+	rootModule := state.RootModule()
+	if rootModule == nil {
+		t.Errorf("root module is nil; want valid object")
+	}
+
+	rootModule.SetLocalValue("foo", cty.StringVal("foo value"))
+	rootModule.SetOutputValue("bar", cty.StringVal("bar value"), false)
+	rootModule.SetOutputValue("secret", cty.StringVal("secret value"), true)
+	rootModule.SetResourceInstanceCurrent(
+		addrs.Resource{
+			Mode: addrs.ManagedResourceMode,
+			Type: "test_thing",
+			Name: "baz",
+		}.Instance(addrs.IntKey(0)),
+		&ResourceInstanceObjectSrc{
+			Status:        ObjectReady,
+			SchemaVersion: 1,
+			AttrsJSON:     []byte(`{"woozles":"confuzles"}`),
+			Private:       []byte("private data"),
+			Dependencies:  []addrs.Referenceable{},
+		},
+		addrs.ProviderConfig{
+			Type: "test",
+		}.Absolute(addrs.RootModuleInstance),
+	)
+	rootModule.SetResourceInstanceCurrent(
+		addrs.Resource{
+			Mode: addrs.ManagedResourceMode,
+			Type: "test_thing",
+			Name: "bar",
+		}.Instance(addrs.IntKey(0)),
+		&ResourceInstanceObjectSrc{
+			Status:        ObjectReady,
+			SchemaVersion: 1,
+			AttrsJSON:     []byte(`{"woozles":"confuzles"}`),
+			Private:       []byte("private data"),
+			Dependencies: []addrs.Referenceable{addrs.Resource{
+				Mode: addrs.ManagedResourceMode,
+				Type: "test_thing",
+				Name: "baz",
+			}},
+		},
+		addrs.ProviderConfig{
+			Type: "test",
+		}.Absolute(addrs.RootModuleInstance),
+	)
+
+	childModule := state.EnsureModule(addrs.RootModuleInstance.Child("child", addrs.NoKey))
+	childModule.SetOutputValue("pizza", cty.StringVal("hawaiian"), false)
+
+	stateCopy := state.DeepCopy()
+	if !state.Equal(stateCopy) {
+		t.Fatalf("\nexpected:\n%q\ngot:\n%q\n", state, stateCopy)
+	}
+}

terraform/eval_diff.go

@@ -448,8 +448,9 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) {
 			// must _also_ record the returned change in the active plan,
 			// which the expression evaluator will use in preference to this
 			// incomplete value recorded in the state.
-			Status: states.ObjectPlanned,
-			Value:  plannedNewVal,
+			Status:  states.ObjectPlanned,
+			Value:   plannedNewVal,
+			Private: plannedPrivate,
 		}
 	}
 
@@ -790,6 +791,7 @@ func (n *EvalDiffDestroy) Eval(ctx EvalContext) (interface{}, error) {
 			Before: state.Value,
 			After:  cty.NullVal(cty.DynamicPseudoType),
 		},
+		Private:      state.Private,
 		ProviderAddr: n.ProviderAddr,
 	}
 

terraform/eval_refresh.go

@@ -55,6 +55,7 @@ func (n *EvalRefresh) Eval(ctx EvalContext) (interface{}, error) {
 	req := providers.ReadResourceRequest{
 		TypeName:   n.Addr.Resource.Type,
 		PriorState: priorVal,
+		Private:    state.Private,
 	}
 
 	provider := *n.Provider
@@ -87,6 +88,7 @@ func (n *EvalRefresh) Eval(ctx EvalContext) (interface{}, error) {
 
 	newState := state.DeepCopy()
 	newState.Value = resp.NewState
+	newState.Private = resp.Private
 
 	// Call post-refresh hook
 	err = ctx.Hook(func(h Hook) (HookAction, error) {