helper/schema: Add Resource type flags for disabling legacy type sy…

…stem protocol flags (#1229)

Reference: #1227

Introduces new `Resource` type `EnableLegacyTypeSystemApplyErrors` and `EnableLegacyTypeSystemPlanErrors` fields that prevent the `PlanResourceChange` and `ApplyResourceChange` protocol operation responses from setting the `UnsafeLegacyTypeSystem` flag. When Terraform encounters the `UnsafeLegacyTypeSystem` flag, it will demote certain data consistency issues from error diagnostics that would immediately prevent Terraform workflows from completing to warning logs, which can be difficult for provider developers to discover. Provider developers are not  generally expected to try enabling these settings unless discovering these data consistency issues upfront is desired before migrating resources to terraform-plugin-framework.

This setting is on the individual resource level because these errors can be quite prevalent in existing terraform-plugin-sdk resources and therefore it would be problematic to not give provider developers more control over the behavior.

The settings are split between plan and apply because the protocol flag is on two separate protocol operations and because certain errors for one operation may be unavoidable, so it leaves provider developers the option to still raise errors for the other operation.

This change set includes new website documentation to discuss these data consistency issues and how to potentially resolve them more in-depth. It is expected that the framework migration guide will be updated to recommend enabling these settings before migrating to help provider developers understand existing issues before migrating. The error resolution section has one particular error to start and it is expected that this section will grow over time as provider developers report the various data consistency errors that Terraform can raise.

.changes/unreleased/ENHANCEMENTS-20230808-111940.yaml

@@ -0,0 +1,8 @@
+kind: ENHANCEMENTS
+body: 'helper/schema: Added `Resource` type `EnableLegacyTypeSystemApplyErrors`
+  field, which will prevent Terraform from demoting data consistency errors
+  to warning logs during `ApplyResourceChange` (`Create`, `Update`, and `Delete`)
+  operations with the resource'
+time: 2023-08-08T11:19:40.137598-04:00
+custom:
+  Issue: "1227"

.changes/unreleased/ENHANCEMENTS-20230808-111941.yaml

@@ -0,0 +1,7 @@
+kind: ENHANCEMENTS
+body: 'helper/schema: Added `Resource` type `EnableLegacyTypeSystemPlanErrors`
+  field, which can be used to prevent Terraform from demoting data consistency
+  errors to warning logs during `PlanResourceChange` operations with the resource'
+time: 2023-08-08T11:19:41.137598-04:00
+custom:
+  Issue: "1227"

.changes/unreleased/NOTES-20230808-103724.yaml

@@ -0,0 +1,12 @@
+kind: NOTES
+body: 'helper/schema: The `Resource` type `EnableApplyLegacyTypeSystemErrors`
+  and `EnablePlanLegacyTypeSystemErrors` fields can be enabled to more easily
+  discover resource data consistency errors which Terraform would normally
+  demote to warning logs. Before enabling the flag in a production release for
+  a resource, the resource should be exhaustively acceptance tested as there may
+  be unrecoverable error situations for practitioners. It is recommended to
+  first enable and test in environments where it is easy to clean up resources,
+  potentially outside of Terraform.'
+time: 2023-08-08T10:37:24.017324-04:00
+custom:
+  Issue: "1227"

helper/schema/grpc_provider.go

@@ -662,20 +662,23 @@ func (s *GRPCProviderServer) PlanResourceChange(ctx context.Context, req *tfprot
 	ctx = logging.InitContext(ctx)
 	resp := &tfprotov5.PlanResourceChangeResponse{}
 
+	res, ok := s.provider.ResourcesMap[req.TypeName]
+	if !ok {
+		resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("unknown resource type: %s", req.TypeName))
+		return resp, nil
+	}
+	schemaBlock := s.getResourceSchemaBlock(req.TypeName)
+
 	// This is a signal to Terraform Core that we're doing the best we can to
 	// shim the legacy type system of the SDK onto the Terraform type system
 	// but we need it to cut us some slack. This setting should not be taken
 	// forward to any new SDK implementations, since setting it prevents us
 	// from catching certain classes of provider bug that can lead to
 	// confusing downstream errors.
-	resp.UnsafeToUseLegacyTypeSystem = true //nolint:staticcheck
-
-	res, ok := s.provider.ResourcesMap[req.TypeName]
-	if !ok {
-		resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("unknown resource type: %s", req.TypeName))
-		return resp, nil
+	if !res.EnableLegacyTypeSystemPlanErrors {
+		//nolint:staticcheck // explicitly for this SDK
+		resp.UnsafeToUseLegacyTypeSystem = true
 	}
-	schemaBlock := s.getResourceSchemaBlock(req.TypeName)
 
 	priorStateVal, err := msgpack.Unmarshal(req.PriorState.MsgPack, schemaBlock.ImpliedType())
 	if err != nil {
@@ -1075,7 +1078,10 @@ func (s *GRPCProviderServer) ApplyResourceChange(ctx context.Context, req *tfpro
 	// forward to any new SDK implementations, since setting it prevents us
 	// from catching certain classes of provider bug that can lead to
 	// confusing downstream errors.
-	resp.UnsafeToUseLegacyTypeSystem = true //nolint:staticcheck
+	if !res.EnableLegacyTypeSystemApplyErrors {
+		//nolint:staticcheck // explicitly for this SDK
+		resp.UnsafeToUseLegacyTypeSystem = true
+	}
 
 	return resp, nil
 }

helper/schema/grpc_provider_test.go

@@ -575,77 +575,113 @@ func TestUpgradeState_flatmapStateMissingMigrateState(t *testing.T) {
 }
 
 func TestPlanResourceChange(t *testing.T) {
-	r := &Resource{
-		SchemaVersion: 4,
-		Schema: map[string]*Schema{
-			"foo": {
-				Type:     TypeInt,
-				Optional: true,
+	t.Parallel()
+
+	testCases := map[string]struct {
+		TestResource                   *Resource
+		ExpectedUnsafeLegacyTypeSystem bool
+	}{
+		"basic": {
+			TestResource: &Resource{
+				SchemaVersion: 4,
+				Schema: map[string]*Schema{
+					"foo": {
+						Type:     TypeInt,
+						Optional: true,
+					},
+				},
+			},
+			ExpectedUnsafeLegacyTypeSystem: true,
+		},
+		"EnableLegacyTypeSystemPlanErrors": {
+			TestResource: &Resource{
+				EnableLegacyTypeSystemPlanErrors: true,
+				Schema: map[string]*Schema{
+					"foo": {
+						Type:     TypeInt,
+						Optional: true,
+					},
+				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: false,
 		},
 	}
 
-	server := NewGRPCProviderServer(&Provider{
-		ResourcesMap: map[string]*Resource{
-			"test": r,
-		},
-	})
+	for name, testCase := range testCases {
+		name, testCase := name, testCase
 
-	schema := r.CoreConfigSchema()
-	priorState, err := msgpack.Marshal(cty.NullVal(schema.ImpliedType()), schema.ImpliedType())
-	if err != nil {
-		t.Fatal(err)
-	}
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
 
-	// A propsed state with only the ID unknown will produce a nil diff, and
-	// should return the propsed state value.
-	proposedVal, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
-		"id": cty.UnknownVal(cty.String),
-	}))
-	if err != nil {
-		t.Fatal(err)
-	}
-	proposedState, err := msgpack.Marshal(proposedVal, schema.ImpliedType())
-	if err != nil {
-		t.Fatal(err)
-	}
+			server := NewGRPCProviderServer(&Provider{
+				ResourcesMap: map[string]*Resource{
+					"test": testCase.TestResource,
+				},
+			})
 
-	config, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
-		"id": cty.NullVal(cty.String),
-	}))
-	if err != nil {
-		t.Fatal(err)
-	}
-	configBytes, err := msgpack.Marshal(config, schema.ImpliedType())
-	if err != nil {
-		t.Fatal(err)
-	}
+			schema := testCase.TestResource.CoreConfigSchema()
+			priorState, err := msgpack.Marshal(cty.NullVal(schema.ImpliedType()), schema.ImpliedType())
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	testReq := &tfprotov5.PlanResourceChangeRequest{
-		TypeName: "test",
-		PriorState: &tfprotov5.DynamicValue{
-			MsgPack: priorState,
-		},
-		ProposedNewState: &tfprotov5.DynamicValue{
-			MsgPack: proposedState,
-		},
-		Config: &tfprotov5.DynamicValue{
-			MsgPack: configBytes,
-		},
-	}
+			// A propsed state with only the ID unknown will produce a nil diff, and
+			// should return the propsed state value.
+			proposedVal, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
+				"id": cty.UnknownVal(cty.String),
+			}))
+			if err != nil {
+				t.Fatal(err)
+			}
+			proposedState, err := msgpack.Marshal(proposedVal, schema.ImpliedType())
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	resp, err := server.PlanResourceChange(context.Background(), testReq)
-	if err != nil {
-		t.Fatal(err)
-	}
+			config, err := schema.CoerceValue(cty.ObjectVal(map[string]cty.Value{
+				"id": cty.NullVal(cty.String),
+			}))
+			if err != nil {
+				t.Fatal(err)
+			}
+			configBytes, err := msgpack.Marshal(config, schema.ImpliedType())
+			if err != nil {
+				t.Fatal(err)
+			}
 
-	plannedStateVal, err := msgpack.Unmarshal(resp.PlannedState.MsgPack, schema.ImpliedType())
-	if err != nil {
-		t.Fatal(err)
-	}
+			testReq := &tfprotov5.PlanResourceChangeRequest{
+				TypeName: "test",
+				PriorState: &tfprotov5.DynamicValue{
+					MsgPack: priorState,
+				},
+				ProposedNewState: &tfprotov5.DynamicValue{
+					MsgPack: proposedState,
+				},
+				Config: &tfprotov5.DynamicValue{
+					MsgPack: configBytes,
+				},
+			}
 
-	if !cmp.Equal(proposedVal, plannedStateVal, valueComparer) {
-		t.Fatal(cmp.Diff(proposedVal, plannedStateVal, valueComparer))
+			resp, err := server.PlanResourceChange(context.Background(), testReq)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			plannedStateVal, err := msgpack.Unmarshal(resp.PlannedState.MsgPack, schema.ImpliedType())
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if !cmp.Equal(proposedVal, plannedStateVal, valueComparer) {
+				t.Fatal(cmp.Diff(proposedVal, plannedStateVal, valueComparer))
+			}
+
+			//nolint:staticcheck // explicitly for this SDK
+			if testCase.ExpectedUnsafeLegacyTypeSystem != resp.UnsafeToUseLegacyTypeSystem {
+				//nolint:staticcheck // explicitly for this SDK
+				t.Fatalf("expected UnsafeLegacyTypeSystem %t, got: %t", testCase.ExpectedUnsafeLegacyTypeSystem, resp.UnsafeToUseLegacyTypeSystem)
+			}
+		})
 	}
 }
 
@@ -730,12 +766,13 @@ func TestPlanResourceChange_bigint(t *testing.T) {
 }
 
 func TestApplyResourceChange(t *testing.T) {
-	testCases := []struct {
-		Description  string
-		TestResource *Resource
+	t.Parallel()
+
+	testCases := map[string]struct {
+		TestResource                   *Resource
+		ExpectedUnsafeLegacyTypeSystem bool
 	}{
-		{
-			Description: "Create",
+		"Create": {
 			TestResource: &Resource{
 				SchemaVersion: 4,
 				Schema: map[string]*Schema{
@@ -749,9 +786,9 @@ func TestApplyResourceChange(t *testing.T) {
 					return nil
 				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: true,
 		},
-		{
-			Description: "CreateContext",
+		"CreateContext": {
 			TestResource: &Resource{
 				SchemaVersion: 4,
 				Schema: map[string]*Schema{
@@ -765,9 +802,9 @@ func TestApplyResourceChange(t *testing.T) {
 					return nil
 				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: true,
 		},
-		{
-			Description: "CreateWithoutTimeout",
+		"CreateWithoutTimeout": {
 			TestResource: &Resource{
 				SchemaVersion: 4,
 				Schema: map[string]*Schema{
@@ -781,9 +818,9 @@ func TestApplyResourceChange(t *testing.T) {
 					return nil
 				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: true,
 		},
-		{
-			Description: "Create_cty",
+		"Create_cty": {
 			TestResource: &Resource{
 				SchemaVersion: 4,
 				Schema: map[string]*Schema{
@@ -806,9 +843,9 @@ func TestApplyResourceChange(t *testing.T) {
 					return nil
 				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: true,
 		},
-		{
-			Description: "CreateContext_SchemaFunc",
+		"CreateContext_SchemaFunc": {
 			TestResource: &Resource{
 				SchemaFunc: func() map[string]*Schema {
 					return map[string]*Schema{
@@ -823,12 +860,32 @@ func TestApplyResourceChange(t *testing.T) {
 					return nil
 				},
 			},
+			ExpectedUnsafeLegacyTypeSystem: true,
+		},
+		"EnableLegacyTypeSystemApplyErrors": {
+			TestResource: &Resource{
+				EnableLegacyTypeSystemApplyErrors: true,
+				Schema: map[string]*Schema{
+					"foo": {
+						Type:     TypeInt,
+						Optional: true,
+					},
+				},
+				CreateContext: func(_ context.Context, rd *ResourceData, _ interface{}) diag.Diagnostics {
+					rd.SetId("bar")
+					return nil
+				},
+			},
+			ExpectedUnsafeLegacyTypeSystem: false,
 		},
 	}
 
-	for _, testCase := range testCases {
-		testCase := testCase
-		t.Run(testCase.Description, func(t *testing.T) {
+	for name, testCase := range testCases {
+		name, testCase := name, testCase
+
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
 			server := NewGRPCProviderServer(&Provider{
 				ResourcesMap: map[string]*Resource{
 					"test": testCase.TestResource,
@@ -892,6 +949,12 @@ func TestApplyResourceChange(t *testing.T) {
 			if id != "bar" {
 				t.Fatalf("incorrect final state: %#v\n", newStateVal)
 			}
+
+			//nolint:staticcheck // explicitly for this SDK
+			if testCase.ExpectedUnsafeLegacyTypeSystem != resp.UnsafeToUseLegacyTypeSystem {
+				//nolint:staticcheck // explicitly for this SDK
+				t.Fatalf("expected UnsafeLegacyTypeSystem %t, got: %t", testCase.ExpectedUnsafeLegacyTypeSystem, resp.UnsafeToUseLegacyTypeSystem)
+			}
 		})
 	}
 }