Merge pull request #20295 from hashicorp/jbardin/apply-error

process state even after provider.Apply errors

builtin/providers/test/resource.go

@@ -1,6 +1,7 @@
 package test
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/hashicorp/terraform/helper/schema"
@@ -123,13 +124,23 @@ func testResource() *schema.Resource {
 					},
 				},
 			},
+			"apply_error": {
+				Type:        schema.TypeString,
+				Optional:    true,
+				Description: "return and error during apply",
+			},
 		},
 	}
 }
 
 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
 }
 

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"),
+			},
+		},
+	})
+}

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)