Merge pull request #21814 from hashicorp/jbardin/private-and-timeout

Private data and timeouts were lost in empty plans

builtin/providers/test/resource_timeout_test.go

@@ -28,6 +28,52 @@ resource "test_resource_timeout" "foo" {
 	})
 }
 
+// start with the default, then modify it
+func TestResourceTimeout_defaults(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_timeout" "foo" {
+	update_delay = "1ms"
+}
+				`),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_timeout" "foo" {
+	update_delay = "2ms"
+	timeouts {
+		update = "3s"
+	}
+}
+				`),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_timeout" "foo" {
+	update_delay = "2s"
+	delete_delay = "2s"
+	timeouts {
+		delete = "3s"
+		update = "3s"
+	}
+}
+				`),
+			},
+			// delete "foo"
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource_timeout" "bar" {
+}
+				`),
+			},
+		},
+	})
+}
+
 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.

helper/plugin/grpc_provider.go

@@ -492,7 +492,12 @@ func (s *GRPCProviderServer) Configure(_ context.Context, req *proto.Configure_R
 }
 
 func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadResource_Request) (*proto.ReadResource_Response, error) {
-	resp := &proto.ReadResource_Response{}
+	resp := &proto.ReadResource_Response{
+		// 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.
+		Private: req.Private,
+	}
 
 	res := s.provider.ResourcesMap[req.TypeName]
 	schemaBlock := s.getResourceSchemaBlock(req.TypeName)
@@ -509,6 +514,15 @@ func (s *GRPCProviderServer) ReadResource(_ context.Context, req *proto.ReadReso
 		return resp, nil
 	}
 
+	private := make(map[string]interface{})
+	if len(req.Private) > 0 {
+		if err := json.Unmarshal(req.Private, &private); err != nil {
+			resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
+			return resp, nil
+		}
+	}
+	instanceState.Meta = private
+
 	newInstanceState, err := res.RefreshWithoutUpgrade(instanceState, s.provider.Meta())
 	if err != nil {
 		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
@@ -551,11 +565,6 @@ 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
 }
 
@@ -645,6 +654,7 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl
 		// description that _shows_ there are no changes. This is always the
 		// prior state, because we force a diff above if this is a new instance.
 		resp.PlannedState = req.PriorState
+		resp.PlannedPrivate = req.PriorPrivate
 		return resp, nil
 	}
 
@@ -705,6 +715,18 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl
 		Msgpack: plannedMP,
 	}
 
+	// encode any timeouts into the diff Meta
+	t := &schema.ResourceTimeout{}
+	if err := t.ConfigDecode(res, cfg); err != nil {
+		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
+		return resp, nil
+	}
+
+	if err := t.DiffEncode(diff); err != nil {
+		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
+		return resp, nil
+	}
+
 	// Now we need to store any NewExtra values, which are where any actual
 	// StateFunc modified config fields are hidden.
 	privateMap := diff.Meta

helper/schema/resource.go

@@ -329,21 +329,13 @@ func (r *Resource) simpleDiff(
 	c *terraform.ResourceConfig,
 	meta interface{}) (*terraform.InstanceDiff, error) {
 
-	t := &ResourceTimeout{}
-	err := t.ConfigDecode(r, c)
-
-	if err != nil {
-		return nil, fmt.Errorf("[ERR] Error decoding timeout: %s", err)
-	}
-
 	instanceDiff, err := schemaMap(r.Schema).Diff(s, c, r.CustomizeDiff, meta, false)
 	if err != nil {
 		return instanceDiff, err
 	}
 
 	if instanceDiff == nil {
-		log.Printf("[DEBUG] Instance Diff is nil in SimpleDiff()")
+		instanceDiff = terraform.NewInstanceDiff()
-		return nil, err
 	}
 
 	// Make sure the old value is set in each of the instance diffs.
@@ -357,10 +349,7 @@ func (r *Resource) simpleDiff(
 		}
 	}
 
-	if err := t.DiffEncode(instanceDiff); err != nil {
+	return instanceDiff, nil
-		log.Printf("[ERR] Error encoding timeout to instance diff: %s", err)
-	}
-	return instanceDiff, err
 }
 
 // Validate validates the resource configuration against the schema.