From 1bfc27817eb00b0b0cf82207be556a801eb5c1ed Mon Sep 17 00:00:00 2001
From: James Bardin <j.bardin@gmail.com>
Date: Mon, 11 Feb 2019 15:35:46 -0500
Subject: [PATCH] process state even after provider.Apply errors

Terraform core expects a sane state even when the provider returns an
error. Make sure at the prior state is always the default value to
return, and then alway attempt to process any state returned by
provider.Apply.
---
 builtin/providers/test/resource.go      | 15 +++++++
 builtin/providers/test/resource_test.go | 53 +++++++++++++++++++++++++
 helper/plugin/grpc_provider.go          | 13 ++++--
 3 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/builtin/providers/test/resource.go b/builtin/providers/test/resource.go
index 9aa384791..490e539f9 100644
--- a/builtin/providers/test/resource.go
+++ b/builtin/providers/test/resource.go
@@ -1,6 +1,7 @@
 package test
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/hashicorp/terraform/helper/schema"
@@ -123,6 +124,11 @@ func testResource() *schema.Resource {
 					},
 				},
 			},
+			"apply_error": {
+				Type:        schema.TypeString,
+				Optional:    true,
+				Description: "return and error during apply",
+			},
 		},
 	}
 }
@@ -130,6 +136,11 @@ func testResource() *schema.Resource {
 func testResourceCreate(d *schema.ResourceData, meta interface{}) error {
 	d.SetId("testId")
 
+	errMsg, _ := d.Get("apply_error").(string)
+	if errMsg != "" {
+		return errors.New(errMsg)
+	}
+
 	// Required must make it through to Create
 	if _, ok := d.GetOk("required"); !ok {
 		return fmt.Errorf("Missing attribute 'required', but it's required!")
@@ -156,6 +167,10 @@ func testResourceRead(d *schema.ResourceData, meta interface{}) error {
 }
 
 func testResourceUpdate(d *schema.ResourceData, meta interface{}) error {
+	errMsg, _ := d.Get("apply_error").(string)
+	if errMsg != "" {
+		return errors.New(errMsg)
+	}
 	return nil
 }
 
diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go
index 05fd5c0c5..2b210d00b 100644
--- a/builtin/providers/test/resource_test.go
+++ b/builtin/providers/test/resource_test.go
@@ -2,6 +2,7 @@ package test
 
 import (
 	"reflect"
+	"regexp"
 	"strings"
 	"testing"
 
@@ -624,3 +625,55 @@ resource "test_resource" "foo" {
 		},
 	})
 }
+
+func TestResource_updateError(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+  required     = "first"
+  required_map = {
+    a = "a"
+  }
+}
+`),
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+  required     = "second"
+  required_map = {
+    a = "a"
+  }
+  apply_error = "update_error"
+}
+`),
+				ExpectError: regexp.MustCompile("update_error"),
+			},
+		},
+	})
+}
+
+func TestResource_applyError(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+  required     = "second"
+  required_map = {
+    a = "a"
+  }
+  apply_error = "apply_error"
+}
+`),
+				ExpectError: regexp.MustCompile("apply_error"),
+			},
+		},
+	})
+}
diff --git a/helper/plugin/grpc_provider.go b/helper/plugin/grpc_provider.go
index 709441b47..d83d484b6 100644
--- a/helper/plugin/grpc_provider.go
+++ b/helper/plugin/grpc_provider.go
@@ -656,7 +656,10 @@ func (s *GRPCProviderServer) PlanResourceChange(_ context.Context, req *proto.Pl
 }
 
 func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.ApplyResourceChange_Request) (*proto.ApplyResourceChange_Response, error) {
-	resp := &proto.ApplyResourceChange_Response{}
+	resp := &proto.ApplyResourceChange_Response{
+		// Start with the existing state as a fallback
+		NewState: req.PriorState,
+	}
 
 	res := s.provider.ResourcesMap[req.TypeName]
 	block := res.CoreConfigSchema()
@@ -753,15 +756,17 @@ func (s *GRPCProviderServer) ApplyResourceChange(_ context.Context, req *proto.A
 	}
 
 	newInstanceState, err := s.provider.Apply(info, priorState, diff)
+	// we record the error here, but continue processing any returned state.
 	if err != nil {
 		resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)
-		return resp, nil
 	}
 
 	newStateVal := cty.NullVal(block.ImpliedType())
 
-	// always return a nul value for destroy
-	if newInstanceState == nil || destroy {
+	// Always return a null value for destroy.
+	// While this is usually indicated by a nil state, check for missing ID or
+	// attributes in the case of a provider failure.
+	if destroy || newInstanceState == nil || newInstanceState.Attributes == nil || newInstanceState.ID == "" {
 		newStateMP, err := msgpack.Marshal(newStateVal, block.ImpliedType())
 		if err != nil {
 			resp.Diagnostics = convert.AppendProtoDiag(resp.Diagnostics, err)